diff mbox series

[pushed] i386: Add ROLW %r, 8 alternative to bswaphi2 patterns [PR126283]

Message ID CAFULd4a6hc0fJW9jn9wwtHPqupp4UFMspMvzeSiNjrT79p+d6A@mail.gmail.com
State Committed
Headers
Series [pushed] i386: Add ROLW %r, 8 alternative to bswaphi2 patterns [PR126283] |

Checks

Context Check Description
linaro-tcwg-bot/tcwg_gcc_build--master-aarch64 fail Patch failed to apply
linaro-tcwg-bot/tcwg_gcc_build--master-arm fail Patch failed to apply
linaro-tcwg-bot/tcwg_simplebootstrap_build--master-aarch64-bootstrap fail Patch failed to apply
linaro-tcwg-bot/tcwg_simplebootstrap_build--master-arm-bootstrap fail Patch failed to apply

Commit Message

Uros Bizjak July 17, 2026, 11:55 a.m. UTC 
  The HImode bswap can be implemented in two ways: using XCHGB %rH,%rL or
ROLW %r,8 where the former does not clobber flags.  Unfortunately, clobber-less
XCHGB form limits available register set to only %ax, %bx, %cx and %dx, where
ROLW can use all registers (but clobbers flags reg).

It is better to use the form that clobbers flags reg and leave insn selection
to the compiler via preferred_for_* machinery.  This choice also makes
"improve register allocation" peephole2 obsolete.

Also, always convert HImode rotate by 8 to bswap pattern, which can also
emit MOVBE instruction when available.

    PR target/126283

gcc/ChangeLog:

    * config/i386/i386.md (bswaphi2): Add flags reg clobber.
    (*bswaphi2_movbe): Add flags reg clobber.  Add ROLW alternative
    and update instruction attributes.  Add preferred_for_size and
    preferred_for_speed attributes.
    (*bswaphi2): Ditto.
    (*bswaphisi2_lowpart): Ditto.
    (XCHGB -> ROLW peephole2 patterns): Remove.
    (ROLW reg,8 -> BSWAP splitter): Update for added flags reg clobber.
    Always split.
    * config/i386/i386-expand.cc (ix86_expand_gfni_bitreverse):
    Update for bswaphi2 change.

Bootstrapped and regression tested on x86_64-linux-gnu {,-m32}.

Uros.

Comments

Maciej W. Rozycki July 20, 2026, 3:12 p.m. UTC | #1 
On Fri, 17 Jul 2026, Uros Bizjak wrote:

> It is better to use the form that clobbers flags reg and leave insn selection
> to the compiler via preferred_for_* machinery.  This choice also makes
> "improve register allocation" peephole2 obsolete.

 Hmm...

> diff --git a/gcc/config/i386/i386.md b/gcc/config/i386/i386.md
> index 3aa8c926094..fbaf28017bb 100644
> --- a/gcc/config/i386/i386.md
> +++ b/gcc/config/i386/i386.md
> @@ -23665,100 +23669,91 @@ (define_expand "bswaphi2"
>  })
>  
>  (define_insn "*bswaphi2_movbe"
> -  [(set (match_operand:HI 0 "nonimmediate_operand" "=Q,r,m")
> -	(bswap:HI (match_operand:HI 1 "nonimmediate_operand" "0,m,r")))]
> +  [(set (match_operand:HI 0 "nonimmediate_operand" "=Q,r,r,m")
> +	(bswap:HI (match_operand:HI 1 "nonimmediate_operand" "0,0,m,r")))
> +   (clobber (reg:CC FLAGS_REG))]
>    "TARGET_MOVBE
>     && !(MEM_P (operands[0]) && MEM_P (operands[1]))"
>    "@
>      xchg{b}\t{%h0, %b0|%b0, %h0}
> +    rol{w}\t{$8, %0|%0, 8}
>      movbe{w}\t{%1, %0|%0, %1}
>      movbe{w}\t{%1, %0|%0, %1}"

 ... would it be possible to tell the middle end via constraints that 
flags aren't necessarily clobbered, i.e.:

   (clobber (match_scratch:CC 2 "=X,Bf,X,X))]

(here and elsewhere) so as to avoid pretending there is an inexistent data 
dependency and consequently possibly improve the code flow?  Or is it a 
completely unreasonable idea that won't work in this case?

  Maciej
Uros Bizjak July 27, 2026, 7:20 p.m. UTC | #2 
On Mon, Jul 20, 2026 at 5:12 PM Maciej W. Rozycki <macro@orcam.me.uk> wrote:
>
> On Fri, 17 Jul 2026, Uros Bizjak wrote:
>
> > It is better to use the form that clobbers flags reg and leave insn selection
> > to the compiler via preferred_for_* machinery.  This choice also makes
> > "improve register allocation" peephole2 obsolete.
>
>  Hmm...
>
> > diff --git a/gcc/config/i386/i386.md b/gcc/config/i386/i386.md
> > index 3aa8c926094..fbaf28017bb 100644
> > --- a/gcc/config/i386/i386.md
> > +++ b/gcc/config/i386/i386.md
> > @@ -23665,100 +23669,91 @@ (define_expand "bswaphi2"
> >  })
> >
> >  (define_insn "*bswaphi2_movbe"
> > -  [(set (match_operand:HI 0 "nonimmediate_operand" "=Q,r,m")
> > -     (bswap:HI (match_operand:HI 1 "nonimmediate_operand" "0,m,r")))]
> > +  [(set (match_operand:HI 0 "nonimmediate_operand" "=Q,r,r,m")
> > +     (bswap:HI (match_operand:HI 1 "nonimmediate_operand" "0,0,m,r")))
> > +   (clobber (reg:CC FLAGS_REG))]
> >    "TARGET_MOVBE
> >     && !(MEM_P (operands[0]) && MEM_P (operands[1]))"
> >    "@
> >      xchg{b}\t{%h0, %b0|%b0, %h0}
> > +    rol{w}\t{$8, %0|%0, 8}
> >      movbe{w}\t{%1, %0|%0, %1}
> >      movbe{w}\t{%1, %0|%0, %1}"
>
>  ... would it be possible to tell the middle end via constraints that
> flags aren't necessarily clobbered, i.e.:
>
>    (clobber (match_scratch:CC 2 "=X,Bf,X,X))]
>
> (here and elsewhere) so as to avoid pretending there is an inexistent data
> dependency and consequently possibly improve the code flow?  Or is it a
> completely unreasonable idea that won't work in this case?

Unfortunately, this won't work, because clobber is also processed well
before reload. The only solution is to have several different insn
patterns, but considering that XCHGB is used only for obsolete
pentium4 does not warrant clobber-less patterns.

Uros.
diff mbox series

Patch

diff --git a/gcc/config/i386/i386-expand.cc b/gcc/config/i386/i386-expand.cc
index fceb958b7e6..bdd75067a00 100644
--- a/gcc/config/i386/i386-expand.cc
+++ b/gcc/config/i386/i386-expand.cc
@@ -28420,8 +28420,11 @@  ix86_expand_gfni_bitreverse (rtx dest, rtx src)
       return;
     }
   if (mode == HImode)
-    target = lowpart_subreg (mode, target, SImode);
-  if (mode == SImode)
+    {
+      target = lowpart_subreg (mode, target, SImode);
+      emit_insn (gen_bswaphi2 (dest, target));
+    }
+  else if (mode == SImode)
     emit_insn (gen_bswapsi2 (dest, target));
   else
     emit_insn (gen_rtx_SET (dest, gen_rtx_BSWAP (mode, target)));
diff --git a/gcc/config/i386/i386.md b/gcc/config/i386/i386.md
index 3aa8c926094..fbaf28017bb 100644
--- a/gcc/config/i386/i386.md
+++ b/gcc/config/i386/i386.md
@@ -19717,12 +19717,14 @@  (define_insn_and_split "*<insn><mode>3_1_slp"
    (set_attr "mode" "<MODE>")])
 
 (define_split
- [(set (match_operand:HI 0 "QIreg_operand")
+ [(set (match_operand:HI 0 "register_operand")
        (any_rotate:HI (match_dup 0) (const_int 8)))
   (clobber (reg:CC FLAGS_REG))]
- "reload_completed
-  && (TARGET_USE_XCHGB || optimize_function_for_size_p (cfun))"
- [(set (match_dup 0) (bswap:HI (match_dup 0)))])
+ ""
+ [(parallel
+    [(set (match_dup 0)
+	  (bswap:HI (match_dup 0)))
+     (clobber (reg:CC FLAGS_REG))])])
 
 ;; Rotations through carry flag
 (define_insn "rcrsi2"
@@ -23656,8 +23658,10 @@  (define_insn "*bswap<mode>2"
    (set_attr "mode" "<MODE>")])
 
 (define_expand "bswaphi2"
-  [(set (match_operand:HI 0 "register_operand")
-	(bswap:HI (match_operand:HI 1 "nonimmediate_operand")))]
+  [(parallel
+     [(set (match_operand:HI 0 "register_operand")
+	   (bswap:HI (match_operand:HI 1 "nonimmediate_operand")))
+      (clobber (reg:CC FLAGS_REG))])]
   ""
 {
   if (!TARGET_MOVBE)
@@ -23665,100 +23669,91 @@  (define_expand "bswaphi2"
 })
 
 (define_insn "*bswaphi2_movbe"
-  [(set (match_operand:HI 0 "nonimmediate_operand" "=Q,r,m")
-	(bswap:HI (match_operand:HI 1 "nonimmediate_operand" "0,m,r")))]
+  [(set (match_operand:HI 0 "nonimmediate_operand" "=Q,r,r,m")
+	(bswap:HI (match_operand:HI 1 "nonimmediate_operand" "0,0,m,r")))
+   (clobber (reg:CC FLAGS_REG))]
   "TARGET_MOVBE
    && !(MEM_P (operands[0]) && MEM_P (operands[1]))"
   "@
     xchg{b}\t{%h0, %b0|%b0, %h0}
+    rol{w}\t{$8, %0|%0, 8}
     movbe{w}\t{%1, %0|%0, %1}
     movbe{w}\t{%1, %0|%0, %1}"
-  [(set_attr "type" "imov")
-   (set_attr "modrm" "*,1,1")
-   (set_attr "prefix_0f" "*,1,1")
-   (set_attr "prefix_extra" "*,1,1")
-   (set_attr "pent_pair" "np,*,*")
-   (set_attr "athlon_decode" "vector,*,*")
-   (set_attr "amdfam10_decode" "double,*,*")
-   (set_attr "c86_decode" "vector,*,*")
-   (set_attr "bdver1_decode" "double,*,*")
-   (set_attr "mode" "QI,HI,HI")])
+  [(set_attr "type" "imov,rotate1,imov,imov")
+   (set_attr "modrm" "*,*,1,1")
+   (set_attr "prefix_0f" "*,*,1,1")
+   (set_attr "prefix_extra" "*,*,1,1")
+   (set_attr "pent_pair" "np,*,*,*")
+   (set_attr "athlon_decode" "vector,*,*,*")
+   (set_attr "amdfam10_decode" "double,*,*,*")
+   (set_attr "c86_decode" "vector,*,*,*")
+   (set_attr "bdver1_decode" "double,*,*,*")
+   (set_attr "mode" "QI,HI,HI,HI")
+   (set (attr "preferred_for_size")
+     (cond [(eq_attr "alternative" "1")
+	      (symbol_ref "false")
+	   ]
+	   (symbol_ref "true")))
+   (set (attr "preferred_for_speed")
+     (cond [(eq_attr "alternative" "1")
+	      (symbol_ref "!TARGET_USE_XCHGB")
+	   ]
+	   (symbol_ref "true")))])
 
 (define_insn "*bswaphi2"
-  [(set (match_operand:HI 0 "register_operand" "=Q")
-	(bswap:HI (match_operand:HI 1 "register_operand" "0")))]
+  [(set (match_operand:HI 0 "register_operand" "=Q,r")
+	(bswap:HI (match_operand:HI 1 "register_operand" "0,0")))
+   (clobber (reg:CC FLAGS_REG))]
   "!TARGET_MOVBE"
-  "xchg{b}\t{%h0, %b0|%b0, %h0}"
-  [(set_attr "type" "imov")
-   (set_attr "pent_pair" "np")
-   (set_attr "athlon_decode" "vector")
-   (set_attr "amdfam10_decode" "double")
-   (set_attr "c86_decode" "vector")
-   (set_attr "bdver1_decode" "double")
-   (set_attr "mode" "QI")])
-
-(define_peephole2
-  [(set (match_operand:HI 0 "general_reg_operand")
-	(bswap:HI (match_dup 0)))]
-  "!(TARGET_USE_XCHGB ||
-     TARGET_PARTIAL_REG_STALL || optimize_function_for_size_p (cfun))
-   && peep2_regno_dead_p (0, FLAGS_REG)"
-  [(parallel [(set (match_dup 0) (rotate:HI (match_dup 0) (const_int 8)))
-	      (clobber (reg:CC FLAGS_REG))])])
+  "@
+    xchg{b}\t{%h0, %b0|%b0, %h0}
+    rol{w}\t{$8, %0|%0, 8}"
+  [(set_attr "type" "imov,rotate1")
+   (set_attr "pent_pair" "np,*")
+   (set_attr "athlon_decode" "vector,*")
+   (set_attr "amdfam10_decode" "double,*")
+   (set_attr "c86_decode" "vector,*")
+   (set_attr "bdver1_decode" "double,*")
+   (set_attr "mode" "QI,HI")
+   (set (attr "preferred_for_size")
+     (cond [(eq_attr "alternative" "1")
+	      (symbol_ref "false")
+	   ]
+	   (symbol_ref "true")))
+   (set (attr "preferred_for_speed")
+     (cond [(eq_attr "alternative" "1")
+	      (symbol_ref "!TARGET_USE_XCHGB")
+	   ]
+	   (symbol_ref "true")))])
 
 (define_insn "bswaphisi2_lowpart"
-  [(set (match_operand:SI 0 "register_operand" "=Q")
-	(ior:SI (and:SI (match_operand:SI 1 "register_operand" "0")
+  [(set (match_operand:SI 0 "register_operand" "=Q,r")
+	(ior:SI (and:SI (match_operand:SI 1 "register_operand" "0,0")
 			(const_int -65536))
 		(lshiftrt:SI (bswap:SI (match_dup 1))
-			     (const_int 16))))]
-  ""
-  "xchg{b}\t{%h0, %b0|%b0, %h0}"
-  [(set_attr "type" "imov")
-   (set_attr "pent_pair" "np")
-   (set_attr "athlon_decode" "vector")
-   (set_attr "amdfam10_decode" "double")
-   (set_attr "c86_decode" "vector")
-   (set_attr "bdver1_decode" "double")
-   (set_attr "mode" "QI")])
-
-(define_peephole2
-  [(set (match_operand:SI 0 "general_reg_operand")
-	(ior:SI (and:SI (match_dup 0)
-			(const_int -65536))
-		(lshiftrt:SI (bswap:SI (match_dup 0))
-			     (const_int 16))))]
-  "!(TARGET_USE_XCHGB ||
-     TARGET_PARTIAL_REG_STALL || optimize_function_for_size_p (cfun))
-   && peep2_regno_dead_p (0, FLAGS_REG)"
-  [(parallel [(set (strict_low_part (match_dup 0))
-				    (rotate:HI (match_dup 0) (const_int 8)))
-	      (clobber (reg:CC FLAGS_REG))])]
-  "operands[0] = gen_lowpart (HImode, operands[0]);")
-
-;; Variant of above peephole2 to improve register allocation.
-(define_peephole2
-  [(set (match_operand:SI 0 "general_reg_operand")
-        (match_operand:SI 1 "register_operand"))
-   (set (match_dup 0)
-	(ior:SI (and:SI (match_dup 0)
-			(const_int -65536))
-		(lshiftrt:SI (bswap:SI (match_dup 0))
 			     (const_int 16))))
-   (set (match_operand:SI 2 "general_reg_operand") (match_dup 0))]
-  "!(TARGET_USE_XCHGB ||
-     TARGET_PARTIAL_REG_STALL || optimize_function_for_size_p (cfun))
-   && peep2_regno_dead_p (0, FLAGS_REG)
-   && peep2_reg_dead_p(3, operands[0])"
-  [(parallel
-    [(set (strict_low_part (match_dup 3))
-	  (rotate:HI (match_dup 3) (const_int 8)))
-     (clobber (reg:CC FLAGS_REG))])]
-{
-  if (!rtx_equal_p (operands[1], operands[2]))
-    emit_move_insn (operands[2], operands[1]);
-  operands[3] = gen_lowpart (HImode, operands[2]);
-})
+   (clobber (reg:CC FLAGS_REG))]
+  ""
+  "@
+    xchg{b}\t{%h0, %b0|%b0, %h0}
+    rol{w}\t{$8, %0|%0, 8}"
+  [(set_attr "type" "imov,rotate1")
+   (set_attr "pent_pair" "np,*")
+   (set_attr "athlon_decode" "vector,*")
+   (set_attr "amdfam10_decode" "double,*")
+   (set_attr "c86_decode" "vector,*")
+   (set_attr "bdver1_decode" "double,*")
+   (set_attr "mode" "QI,HI")
+   (set (attr "preferred_for_size")
+     (cond [(eq_attr "alternative" "1")
+	      (symbol_ref "false")
+	   ]
+	   (symbol_ref "true")))
+   (set (attr "preferred_for_speed")
+     (cond [(eq_attr "alternative" "1")
+	      (symbol_ref "!TARGET_USE_XCHGB")
+	   ]
+	   (symbol_ref "true")))])
 
 (define_expand "bitreverse<mode>2"
   [(set (match_operand:SWIDWI 0 "register_operand")
diff --git a/gcc/testsuite/gcc.target/i386/pr126283.c b/gcc/testsuite/gcc.target/i386/pr126283.c
new file mode 100644
index 00000000000..5f9a03225e2
--- /dev/null
+++ b/gcc/testsuite/gcc.target/i386/pr126283.c
@@ -0,0 +1,9 @@ 
+/* PR target/126283 */
+/* { dg-do compile } */
+/* { dg-options "-O2 -masm=att" } */
+/* { dg-additional-options "-mregparm=1" { target ia32 } } */
+/* { dg-final { scan-assembler-not "movl\[\\t \]+" } } */
+
+int f (short x) {
+  return __builtin_bswap16(x);
+}
diff --git a/gcc/testsuite/gcc.target/i386/xchg-1.c b/gcc/testsuite/gcc.target/i386/xchg-1.c
index e81fe49cd0b..ea9fc7a8b14 100644
--- a/gcc/testsuite/gcc.target/i386/xchg-1.c
+++ b/gcc/testsuite/gcc.target/i386/xchg-1.c
@@ -6,4 +6,4 @@  unsigned short good(unsigned short a)
        return (a >> 8 | a << 8);
 }
 
-/* { dg-final { scan-assembler "rol" } } */
+/* { dg-final { scan-assembler "\[ \t\]xchg" } } */