From 92142019b6cd0cf1fe483203cf3ec451a9848a42 Mon Sep 17 00:00:00 2001 From: Jakub Jelinek Date: Mon, 10 Feb 2025 10:40:22 +0100 Subject: [PATCH] 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" [(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) && INTVAL (operands[2]) >= (optimize_function_for_size_p (cfun) ? 8 : 32)) : !memory_operand (operands[1], 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 define_insn and all the splitters to it and its comparisons/conditional moves/setXX. 2025-02-10 Jakub Jelinek PR target/118623 * config/i386/i386.md (*bt): Represent bt as compare:CCC of const0_rtx and zero_extract rather than zero_extract and const0_rtx. (*bt_mask): Likewise. (*jcc_bt): Likewise. Use LTU and GEU as flags test instead of EQ and NE. (*jcc_bt_mask): Likewise. (*jcc_bt_mask_1): Likewise. (Help combine recognize bt followed by cmov splitter): Likewise. (*bt_setcqi): Likewise. (*bt_setncqi): Likewise. (*bt_setnc): Likewise. (*bt_setncqi_2): Likewise. (*bt_setc_mask): Likewise. * gcc.c-torture/execute/pr118623.c: New test. --- gcc/config/i386/i386.md | 66 +++++++++---------- .../gcc.c-torture/execute/pr118623.c | 23 +++++++ 2 files changed, 56 insertions(+), 33 deletions(-) create mode 100644 gcc/testsuite/gcc.c-torture/execute/pr118623.c diff --git a/gcc/config/i386/i386.md b/gcc/config/i386/i386.md index 27cd6c117b4..8575fbf40fe 100644 --- a/gcc/config/i386/i386.md +++ b/gcc/config/i386/i386.md @@ -19124,11 +19124,11 @@ (define_insn "*bt" [(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,")) - (const_int 0)))] + (match_operand:QI 1 "nonmemory_operand" "q,"))))] "" { switch (get_attr_mode (insn)) @@ -19155,14 +19155,14 @@ (define_insn_and_split "*bt_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 (mode)-1)) == GET_MODE_BITSIZE (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" @@ -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_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_setnc" [(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_setc_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 index 00000000000..bd14e1ce322 --- /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); +}