i386: Change RTL representation of bt[lq] [PR118623]

The following testcase is miscompiled because of RTL represententation
of bt{l,q} insn followed by e.g. j{c,nc} being misleading to what it
actually does.
Let's look e.g. at
(define_insn_and_split "*jcc_bt<mode>"
  [(set (pc)
        (if_then_else (match_operator 0 "bt_comparison_operator"
                        [(zero_extract:SWI48
                           (match_operand:SWI48 1 "nonimmediate_operand")
                           (const_int 1)
                           (match_operand:QI 2 "nonmemory_operand"))
                         (const_int 0)])
                      (label_ref (match_operand 3))
                      (pc)))
   (clobber (reg:CC FLAGS_REG))]
  "(TARGET_USE_BT || optimize_function_for_size_p (cfun))
   && (CONST_INT_P (operands[2])
       ? (INTVAL (operands[2]) < GET_MODE_BITSIZE (<MODE>mode)
          && INTVAL (operands[2])
               >= (optimize_function_for_size_p (cfun) ? 8 : 32))
       : !memory_operand (operands[1], <MODE>mode))
   && ix86_pre_reload_split ()"
  "#"
  "&& 1"
  [(set (reg:CCC FLAGS_REG)
        (compare:CCC
          (zero_extract:SWI48
            (match_dup 1)
            (const_int 1)
            (match_dup 2))
          (const_int 0)))
   (set (pc)
        (if_then_else (match_op_dup 0 [(reg:CCC FLAGS_REG) (const_int 0)])
                      (label_ref (match_dup 3))
                      (pc)))]
{
  operands[0] = shallow_copy_rtx (operands[0]);
  PUT_CODE (operands[0], reverse_condition (GET_CODE (operands[0])));
})
The define_insn part in RTL describes exactly what it does,
jumps to op3 if bit op2 in op1 is set (for op0 NE) or not set (for op0 EQ).
The problem is with what it splits into.
put_condition_code %C1 for CCCmode comparisons emits c for EQ and LTU,
nc for NE and GEU and ICEs otherwise.
CCCmode is used mainly for carry out of add/adc, borrow out of sub/sbb,
in those cases e.g. for add we have
(set (reg:CCC flags) (compare:CCC (plus:M x y) x))
and use (ltu (reg:CCC flags) (const_int 0)) for carry set and
(geu (reg:CCC flags) (const_int 0)) for carry not set.  These cases
model in RTL what is actually happening, compare in infinite precision
x from the result of finite precision addition in M mode and if it is
less than unsigned (i.e. overflow happened), carry is set.
Another use of CCCmode is in UNSPEC_* patterns, those are used with
(eq (reg:CCC flags) (const_int 0)) for carry set and ne for unset,
given the UNSPEC no big deal, the middle-end doesn't know what means
set or unset.
But for the bt{l,q}; j{c,nc} case the above splits it into
(set (reg:CCC flags) (compare:CCC (zero_extract) (const_int 0)))
for bt and
(set (pc) (if_then_else (eq (reg:CCC flags) (const_int 0)) (label_ref) (pc)))
for the bit set case (so that the jump expands to jc) and ne for
the bit not set case (so that the jump expands to jnc).
Similarly for the different splitters for cmov and set{c,nc} etc.
The problem is that when the middle-end reads this RTL, it feels
the exact opposite to it.  If zero_extract is 1, flags is set
to comparison of 1 and 0 and that would mean using ne ne in the
if_then_else, and vice versa.

So, in order to better describe in RTL what is actually happening,
one possibility would be to swap the behavior of put_condition_code
and use NE + LTU -> c and EQ + GEU -> nc rather than the current
EQ + LTU -> c and NE + GEU -> nc; and adjust everything.  The
following patch uses a more limited approach, instead of representing
bt{l,q}; j{c,nc} case as written above it uses
(set (reg:CCC flags) (compare:CCC (const_int 0) (zero_extract)))
and
(set (pc) (if_then_else (ltu (reg:CCC flags) (const_int 0)) (label_ref) (pc)))
which uses the existing put_condition_code but describes what the
insns actually do in RTL clearly.  If zero_extract is 1,
then flags are LTU, 0U < 1U, if zero_extract is 0, then flags are GEU,
0U >= 0U.  The patch adjusts the *bt<mode> define_insn and all the
splitters to it and its comparisons/conditional moves/setXX.

2025-02-10  Jakub Jelinek  <jakub@redhat.com>

	PR target/118623
	* config/i386/i386.md (*bt<mode>): Represent bt as
	compare:CCC of const0_rtx and zero_extract rather than
	zero_extract and const0_rtx.
	(*bt<SWI48:mode>_mask): Likewise.
	(*jcc_bt<mode>): Likewise.  Use LTU and GEU as flags test
	instead of EQ and NE.
	(*jcc_bt<mode>_mask): Likewise.
	(*jcc_bt<SWI48:mode>_mask_1): Likewise.
	(Help combine recognize bt followed by cmov splitter): Likewise.
	(*bt<mode>_setcqi): Likewise.
	(*bt<mode>_setncqi): Likewise.
	(*bt<mode>_setnc<mode>): Likewise.
	(*bt<mode>_setncqi_2): Likewise.
	(*bt<mode>_setc<mode>_mask): Likewise.

	* gcc.c-torture/execute/pr118623.c: New test.

diff --git a/gcc/config/i386/i386.md b/gcc/config/i386/i386.md
index 27cd6c117b4eb511553ee06f46ec1eba7b2e6f0c..8575fbf40fedc33b9d4f01cb3af79e6d30c9955a 100644 (file)
--- a/gcc/config/i386/i386.md
+++ b/gcc/config/i386/i386.md
@@ -19124,11 +19124,11 @@
 (define_insn "*bt<mode>"
   [(set (reg:CCC FLAGS_REG)
        (compare:CCC
+         (const_int 0)
          (zero_extract:SWI48
            (match_operand:SWI48 0 "nonimmediate_operand" "r,m")
            (const_int 1)
-           (match_operand:QI 1 "nonmemory_operand" "q<S>,<S>"))
-         (const_int 0)))]
+           (match_operand:QI 1 "nonmemory_operand" "q<S>,<S>"))))]
   ""
 {
   switch (get_attr_mode (insn))
@@ -19155,14 +19155,14 @@
 (define_insn_and_split "*bt<SWI48:mode>_mask"
   [(set (reg:CCC FLAGS_REG)
         (compare:CCC
+         (const_int 0)
           (zero_extract:SWI48
             (match_operand:SWI48 0 "nonimmediate_operand" "r,m")
             (const_int 1)
            (subreg:QI
              (and:SWI248
                (match_operand:SWI248 1 "register_operand")
-               (match_operand 2 "const_int_operand")) 0))
-          (const_int 0)))]
+               (match_operand 2 "const_int_operand")) 0))))]
   "TARGET_USE_BT
    && (INTVAL (operands[2]) & (GET_MODE_BITSIZE (<SWI48:MODE>mode)-1))
       == GET_MODE_BITSIZE (<SWI48:MODE>mode)-1
@@ -19171,8 +19171,8 @@
   "&& 1"
   [(set (reg:CCC FLAGS_REG)
         (compare:CCC
-         (zero_extract:SWI48 (match_dup 0) (const_int 1) (match_dup 1))
-         (const_int 0)))]
+        (const_int 0)
+        (zero_extract:SWI48 (match_dup 0) (const_int 1) (match_dup 1))))]
   "operands[1] = gen_lowpart (QImode, operands[1]);")
 
 (define_insn_and_split "*jcc_bt<mode>"
@@ -19197,18 +19197,18 @@
   "&& 1"
   [(set (reg:CCC FLAGS_REG)
        (compare:CCC
+         (const_int 0)
          (zero_extract:SWI48
            (match_dup 1)
            (const_int 1)
-           (match_dup 2))
-         (const_int 0)))
+           (match_dup 2))))
    (set (pc)
        (if_then_else (match_op_dup 0 [(reg:CCC FLAGS_REG) (const_int 0)])
                      (label_ref (match_dup 3))
                      (pc)))]
 {
   operands[0] = shallow_copy_rtx (operands[0]);
-  PUT_CODE (operands[0], reverse_condition (GET_CODE (operands[0])));
+  PUT_CODE (operands[0], GET_CODE (operands[0]) == NE ? LTU : GEU);
 })
 
 ;; Avoid useless masking of bit offset operand.
@@ -19233,18 +19233,18 @@
   "&& 1"
   [(set (reg:CCC FLAGS_REG)
        (compare:CCC
+         (const_int 0)
          (zero_extract:SWI48
            (match_dup 1)
            (const_int 1)
-           (match_dup 2))
-         (const_int 0)))
+           (match_dup 2))))
    (set (pc)
        (if_then_else (match_op_dup 0 [(reg:CCC FLAGS_REG) (const_int 0)])
                      (label_ref (match_dup 4))
                      (pc)))]
 {
   operands[0] = shallow_copy_rtx (operands[0]);
-  PUT_CODE (operands[0], reverse_condition (GET_CODE (operands[0])));
+  PUT_CODE (operands[0], GET_CODE (operands[0]) == NE ? LTU : GEU);
 })
 
 ;; Avoid useless masking of bit offset operand.
@@ -19270,18 +19270,18 @@
   "&& 1"
   [(set (reg:CCC FLAGS_REG)
        (compare:CCC
+         (const_int 0)
          (zero_extract:SWI48
            (match_dup 1)
            (const_int 1)
-           (match_dup 2))
-         (const_int 0)))
+           (match_dup 2))))
    (set (pc)
        (if_then_else (match_op_dup 0 [(reg:CCC FLAGS_REG) (const_int 0)])
                      (label_ref (match_dup 4))
                      (pc)))]
 {
   operands[0] = shallow_copy_rtx (operands[0]);
-  PUT_CODE (operands[0], reverse_condition (GET_CODE (operands[0])));
+  PUT_CODE (operands[0], GET_CODE (operands[0]) == NE ? LTU : GEU);
   operands[2] = gen_lowpart (QImode, operands[2]);
 })
 
@@ -19302,10 +19302,10 @@
    && ix86_pre_reload_split ()"
   [(set (reg:CCC FLAGS_REG)
        (compare:CCC
-        (zero_extract:SWI48 (match_dup 1) (const_int 1) (match_dup 2))
-        (const_int 0)))
+        (const_int 0)
+        (zero_extract:SWI48 (match_dup 1) (const_int 1) (match_dup 2))))
    (set (match_dup 0)
-       (if_then_else:SWI248 (eq (reg:CCC FLAGS_REG) (const_int 0))
+       (if_then_else:SWI248 (ltu (reg:CCC FLAGS_REG) (const_int 0))
                             (match_dup 3)
                             (match_dup 4)))]
 {
@@ -19326,10 +19326,10 @@
   "&& 1"
   [(set (reg:CCC FLAGS_REG)
         (compare:CCC
-         (zero_extract:SWI48 (match_dup 1) (const_int 1) (match_dup 2))
-         (const_int 0)))
+        (const_int 0)
+        (zero_extract:SWI48 (match_dup 1) (const_int 1) (match_dup 2))))
    (set (match_dup 0)
-        (eq:QI (reg:CCC FLAGS_REG) (const_int 0)))])
+       (ltu:QI (reg:CCC FLAGS_REG) (const_int 0)))])
 
 ;; Help combine recognize bt followed by setnc
 (define_insn_and_split "*bt<mode>_setncqi"
@@ -19346,10 +19346,10 @@
   "&& 1"
   [(set (reg:CCC FLAGS_REG)
         (compare:CCC
-         (zero_extract:SWI48 (match_dup 1) (const_int 1) (match_dup 2))
-         (const_int 0)))
+        (const_int 0)
+        (zero_extract:SWI48 (match_dup 1) (const_int 1) (match_dup 2))))
    (set (match_dup 0)
-        (ne:QI (reg:CCC FLAGS_REG) (const_int 0)))])
+       (geu:QI (reg:CCC FLAGS_REG) (const_int 0)))])
 
 (define_insn_and_split "*bt<mode>_setnc<mode>"
   [(set (match_operand:SWI48 0 "register_operand")
@@ -19364,10 +19364,10 @@
   "&& 1"
   [(set (reg:CCC FLAGS_REG)
         (compare:CCC
-         (zero_extract:SWI48 (match_dup 1) (const_int 1) (match_dup 2))
-         (const_int 0)))
+        (const_int 0)
+        (zero_extract:SWI48 (match_dup 1) (const_int 1) (match_dup 2))))
    (set (match_dup 3)
-        (ne:QI (reg:CCC FLAGS_REG) (const_int 0)))
+       (geu:QI (reg:CCC FLAGS_REG) (const_int 0)))
    (set (match_dup 0) (zero_extend:SWI48 (match_dup 3)))]
   "operands[3] = gen_reg_rtx (QImode);")
 
@@ -19386,10 +19386,10 @@
   "&& 1"
   [(set (reg:CCC FLAGS_REG)
         (compare:CCC
-         (zero_extract:SWI48 (match_dup 1) (const_int 1) (match_dup 2))
-         (const_int 0)))
+         (const_int 0)
+         (zero_extract:SWI48 (match_dup 1) (const_int 1) (match_dup 2))))
    (set (match_dup 0)
-        (ne:QI (reg:CCC FLAGS_REG) (const_int 0)))])
+       (geu:QI (reg:CCC FLAGS_REG) (const_int 0)))])
 
 ;; Help combine recognize bt followed by setc
 (define_insn_and_split "*bt<mode>_setc<mode>_mask"
@@ -19410,10 +19410,10 @@
   "&& 1"
   [(set (reg:CCC FLAGS_REG)
         (compare:CCC
-         (zero_extract:SWI48 (match_dup 1) (const_int 1) (match_dup 2))
-         (const_int 0)))
+         (const_int 0)
+         (zero_extract:SWI48 (match_dup 1) (const_int 1) (match_dup 2))))
    (set (match_dup 3)
-        (eq:QI (reg:CCC FLAGS_REG) (const_int 0)))
+       (ltu:QI (reg:CCC FLAGS_REG) (const_int 0)))
    (set (match_dup 0) (zero_extend:SWI48 (match_dup 3)))]
 {
   operands[2] = gen_lowpart (QImode, operands[2]);

diff --git a/gcc/testsuite/gcc.c-torture/execute/pr118623.c b/gcc/testsuite/gcc.c-torture/execute/pr118623.c
new file mode 100644 (file)
index 0000000..bd14e1c
--- /dev/null
+++ b/gcc/testsuite/gcc.c-torture/execute/pr118623.c
@@ -0,0 +1,23 @@
+/* PR target/118623 */
+
+static int
+foo (int x, int y)
+{
+  int a = 1 << x;
+  if (y & a)
+    return 0;
+  return 5;
+}
+
+__attribute__((noipa)) void
+bar (int x)
+{
+  if (((foo (x - 50, x) + x + x) & 1) == 0)
+    __builtin_abort ();
+}
+
+int
+main ()
+{
+  bar (63);
+}