Hi Richard, Thank you for your review and apologies for the late reply, I was travelling for some time before and after Cauldron and forgot about this patch.
On 9/25/26 1:24 AM, Richard Sandiford wrote:
David Faust <[email protected]> writes:Extend cbranch, cstore and cmov patterns to accomodate TImode operands. This leads to potentially much better code for 128-bit integer compares and conditional moves by making use of the conditional instructions. For example, (result = a < b ? c : d) where all operands are uint128 may now be implemented via cmp+sbcs for the compare and a pair of csel for the move. Andrea wrote most of the patch a while ago in the linked PR, I just got cmov working and fixed an error that fell out of testing. Bootstrapped and regtested on aarch64-linux-gnu. No known regressions. gcc/ PR target/116509 * config/aarch64/aarch64-protos.h (aarch64_gen_compare_reg): Add new proto with pointer for insn code. * config/aarch64/aarch64.cc (aarch64_gen_compare_reg): New version which accepts a pointer for the insn code and possibly changes it. Extend handling for TImode. Existing version now wrapps this one. * config/aarch64/aarch64.md (cbranch<GPI:mode>4): Change to... (cbranch<mode>4): ... this. Handle TImode inputs as well. (cstore<mode>4): Accept and handle TImode inputs. ("*cmovti_insn"): New. * config/aarch64/iterators.md (GPI_TI): New mode iterator. gcc/testsuite/ PR target/116509 * gcc.target/aarch64/pr116509-1.c: New test. * gcc.target/aarch64/pr116509-2.c: New test. * gcc.target/aarch64/pr116509-3.c: New test.It seems like the new pattern is a normal
I think you mean that the tests should be named for the insn rather than the PR, right?
Co-authored-by: Andrea Pinski <[email protected]> Signed-off-by: David Faust <[email protected]> --- gcc/config/aarch64/aarch64-protos.h | 1 + gcc/config/aarch64/aarch64.cc | 68 +++++++++-- gcc/config/aarch64/aarch64.md | 59 ++++++++-- gcc/config/aarch64/iterators.md | 3 + gcc/testsuite/gcc.target/aarch64/pr116509-1.c | 55 +++++++++ gcc/testsuite/gcc.target/aarch64/pr116509-2.c | 75 ++++++++++++ gcc/testsuite/gcc.target/aarch64/pr116509-3.c | 110 ++++++++++++++++++ 7 files changed, 352 insertions(+), 19 deletions(-) create mode 100644 gcc/testsuite/gcc.target/aarch64/pr116509-1.c create mode 100644 gcc/testsuite/gcc.target/aarch64/pr116509-2.c create mode 100644 gcc/testsuite/gcc.target/aarch64/pr116509-3.c diff --git a/gcc/config/aarch64/aarch64-protos.h b/gcc/config/aarch64/aarch64-protos.h index e8ae3d42794..d21bcbcf80f 100644 --- a/gcc/config/aarch64/aarch64-protos.h +++ b/gcc/config/aarch64/aarch64-protos.h @@ -1121,6 +1121,7 @@ void aarch64_gen_unlikely_cbranch (enum rtx_code, machine_mode cc_mode, bool aarch64_legitimate_address_p (machine_mode, rtx, bool, aarch64_addr_query_type = ADDR_QUERY_M); machine_mode aarch64_select_cc_mode (RTX_CODE, rtx, rtx); +rtx aarch64_gen_compare_reg (RTX_CODE*, rtx, rtx); rtx aarch64_gen_compare_reg (RTX_CODE, rtx, rtx); rtx aarch64_gen_compare_split_imm24 (rtx, rtx, rtx); bool aarch64_maxmin_plus_const (rtx_code, rtx *, bool); diff --git a/gcc/config/aarch64/aarch64.cc b/gcc/config/aarch64/aarch64.cc index f9d906f449f..80e218da5d8 100644 --- a/gcc/config/aarch64/aarch64.cc +++ b/gcc/config/aarch64/aarch64.cc @@ -3147,7 +3147,7 @@ emit_set_insn (rtx x, rtx y) /* X and Y are two things to compare using CODE. Emit the compare insn and return the rtx for register 0 in the proper mode. */ rtx -aarch64_gen_compare_reg (RTX_CODE code, rtx x, rtx y) +aarch64_gen_compare_reg (RTX_CODE *code, rtx x, rtx y)The comment should describe how CODE can change.
Yes, good point. Will do.
{ machine_mode cmp_mode = GET_MODE (x); machine_mode cc_mode; @@ -3155,30 +3155,76 @@ aarch64_gen_compare_reg (RTX_CODE code, rtx x, rtx y)if (cmp_mode == TImode){ - gcc_assert (code == NE); + switch (*code) + { + case GTU: + std::swap (x, y); + *code = LTU; + break; + case LEU: + std::swap (x, y); + *code = GEU; + break; + case GT: + std::swap (x, y); + *code = LT; + break; + case LE: + std::swap (x, y); + *code = GE; + break; + default: + ; + } + rtx x_lo = operand_subword_force (x, 0, TImode); + rtx y_lo = operand_subword_force (y, 0, TImode); + rtx x_hi = operand_subword_force (x, 1, TImode); + rtx y_hi = operand_subword_force (y, 1, TImode);+ x_lo = force_reg (DImode, x_lo);+ if (!aarch64_plus_operand (y_lo, DImode)) + y_lo = force_reg (DImode, y_lo); cc_mode = CCmode; cc_reg = gen_rtx_REG (cc_mode, CC_REGNUM);- rtx x_lo = operand_subword (x, 0, 0, TImode);- rtx y_lo = operand_subword (y, 0, 0, TImode); emit_set_insn (cc_reg, gen_rtx_COMPARE (cc_mode, x_lo, y_lo)); - - rtx x_hi = operand_subword (x, 1, 0, TImode); - rtx y_hi = operand_subword (y, 1, 0, TImode); - emit_insn (gen_ccmpccdi (cc_reg, cc_reg, x_hi, y_hi, - gen_rtx_EQ (cc_mode, cc_reg, const0_rtx), - GEN_INT (AARCH64_EQ))); + if (*code == NE || *code == EQ) + emit_insn (gen_ccmpccdi (cc_reg, cc_reg, x_hi, y_hi, + gen_rtx_EQ (cc_mode, cc_reg, const0_rtx), + GEN_INT (AARCH64_EQ))); + else + { + /* FIXME: Remove this temp register, use xzr. */It would be good to fix the FIXME :) There is already a "negv<GPI:mode>_cmp_only" pattern that does something similar for NEGS. The same approach should work here.
Ah, thanks for the pointer. I'm new to working with aarch64 and was a little surprised to not see an obvious way to use xzr here.
+ rtx tmp = gen_reg_rtx (DImode); + x_hi = force_reg (DImode, x_hi); + y_hi = force_reg (DImode, y_hi); + if (unsigned_condition_p (*code)) + emit_insn (gen_usubdi3_carryinC (tmp, x_hi, y_hi)); + else + emit_insn (gen_subdi3_carryinV (tmp, x_hi, y_hi)); + } } else { - cc_mode = SELECT_CC_MODE (code, x, y); + cc_mode = SELECT_CC_MODE (*code, x, y); cc_reg = gen_rtx_REG (cc_mode, CC_REGNUM); emit_set_insn (cc_reg, gen_rtx_COMPARE (cc_mode, x, y)); } return cc_reg; }+/* X and Y are two things to compare using CODE. Emit the compare insn and+ return the rtx for register 0 in the proper mode. + CODE cannot not change. */ +rtx +aarch64_gen_compare_reg (RTX_CODE code, rtx x, rtx y) +{ + RTX_CODE old_code = code; + rtx res = aarch64_gen_compare_reg (&code, x, y); + gcc_checking_assert (code == old_code); + return res; +} + /* Similarly, but maybe zero-extend Y if Y_MODE < SImode. */static rtxdiff --git a/gcc/config/aarch64/aarch64.md b/gcc/config/aarch64/aarch64.md index 302658c970d..1d1550543d8 100644 --- a/gcc/config/aarch64/aarch64.md +++ b/gcc/config/aarch64/aarch64.md @@ -818,23 +818,27 @@ (define_constants ;; 4) Otherwise, emit a CMP+B<cond> sequence. ;; --------------------------------------------------------------------(define_expand "cbranch<GPI:mode>4"+(define_expand "cbranch<mode>4" [(set (pc) (if_then_else (match_operator 0 "aarch64_comparison_operator" - [(match_operand:GPI 1 "register_operand") - (match_operand:GPI 2 "aarch64_plus_operand")]) + [(match_operand:GPI_TI 1 "register_operand") + (match_operand:GPI_TI 2 "aarch64_plus_operand")]) (label_ref (match_operand 3)) (pc)))] "" { - if (TARGET_CMPBR && aarch64_cb_rhs (GET_CODE (operands[0]), operands[2])) + if (<MODE>mode != TImode + && TARGET_CMPBR + && aarch64_cb_rhs (GET_CODE (operands[0]), operands[2])) { /* The branch is supported natively. */ } else { - operands[1] = aarch64_gen_compare_reg (GET_CODE (operands[0]), + rtx_code code = GET_CODE (operands[0]); + operands[1] = aarch64_gen_compare_reg (&code, operands[1], operands[2]); operands[2] = const0_rtx; + PUT_CODE (operands[0], code); } } ) @@ -4780,13 +4784,15 @@ (define_expand "spaceship<mode>4" (define_expand "cstore<mode>4" [(set (match_operand:SI 0 "register_operand") (match_operator:SI 1 "aarch64_comparison_operator" - [(match_operand:GPI 2 "register_operand") - (match_operand:GPI 3 "aarch64_plus_operand")]))] + [(match_operand:GPI_TI 2 "register_operand") + (match_operand:GPI_TI 3 "aarch64_plus_operand")]))] "" " - operands[2] = aarch64_gen_compare_reg (GET_CODE (operands[1]), operands[2], + rtx_code code = GET_CODE (operands[1]); + operands[2] = aarch64_gen_compare_reg (&code, operands[2], operands[3]); operands[3] = const0_rtx; + PUT_CODE (operands[1], code); " )@@ -4896,6 +4902,43 @@ (define_insn "*cmov<mode>_insn"} )+;; 128-bit version of above.+(define_insn_and_split "*cmovti_insn" + [(set (match_operand:TI 0 "register_operand") + (if_then_else:TI + (match_operator 1 "aarch64_comparison_operator" + [(match_operand 2 "cc_register") (const_int 0)]) + (match_operand:TI 3 "register_operand") + (match_operand:TI 4 "register_operand")))] + "" + "#" + "can_create_pseudo_p ()"This is an ICE trap. A define_insn should have constraints and allow splits at any time, including after RA. An alternative would to be to split this directly in cstore<mode>4.
Yes, you're right. I will find a way to avoid it.
+ [(set (match_operand 5) + (if_then_else:DI (match_dup 1) (match_operand 6) (match_operand 7))) + (set (match_operand 8) + (if_then_else:DI (match_dup 1) (match_operand 9) (match_operand 10)))] + { + rtx dst = operands[0]; + rtx a = operands[3]; + rtx b = operands[4]; + if (!REG_P (dst)) + dst = gen_reg_rtx (TImode);Are you sure that this works? I couldn't see anything that copies the new dst back to operands[0].
'dst' is broken into highpart and lowpart, which must be the two constituent 64-bit regs (operands 5 and 8) and are set separately by the output RTL. It does work, and I've tested a few admittedly simple sample programs to sanity check that it works correctly and that the result after this cmoveti can be used by further computations correctly. But honestly looking at this again after some time I'm not sure I can convince myself of the soundness of why it works. Something still feels a little off, so I will keep looking. Thanks, David
Thanks, Richard+ if (!REG_P (a)) + a = force_reg (TImode, a); + if (!REG_P (b)) + b = force_reg (TImode, b); + + operands[5] = gen_lowpart (DImode, dst); + operands[6] = gen_lowpart (DImode, a); + operands[7] = gen_lowpart (DImode, b); + operands[8] = gen_highpart (DImode, dst); + operands[9] = gen_highpart (DImode, a); + operands[10] = gen_highpart (DImode, b); + } + [(set_attr "type" "csel")] +) + + ;; zero_extend version of above (define_insn "*cmovsi_insn_uxtw" [(set (match_operand:DI 0 "register_operand") diff --git a/gcc/config/aarch64/iterators.md b/gcc/config/aarch64/iterators.md index 8ed91d021f0..e61201d6509 100644 --- a/gcc/config/aarch64/iterators.md +++ b/gcc/config/aarch64/iterators.md @@ -29,6 +29,9 @@ (define_mode_iterator CCFP_CCFPE [CCFP CCFPE]) ;; Iterator for General Purpose Integer registers (32- and 64-bit modes) (define_mode_iterator GPI [SI DI])+;; Iterator for General Purpose Integer registers plus TI+(define_mode_iterator GPI_TI [SI DI TI]) + ;; Iterator for HI, SI, DI, some instructions can only work on these modes. (define_mode_iterator GPI_I16 [(HI "TARGET_FP_F16INST") SI DI])diff --git a/gcc/testsuite/gcc.target/aarch64/pr116509-1.c b/gcc/testsuite/gcc.target/aarch64/pr116509-1.cnew file mode 100644 index 00000000000..040c5f25745 --- /dev/null +++ b/gcc/testsuite/gcc.target/aarch64/pr116509-1.c @@ -0,0 +1,55 @@ +/* { dg-do compile { target int128 } } */ +/* { dg-options { "-O2" } } */ + +/* PR target/116509. + 128-bit int compares should be handled by cmp + ccmp/sbcs. */ + +/* { dg-final { scan-assembler-not "b\\." } } */ +/* { dg-final { scan-assembler-times "\tcmp\t" 10 } } */ +/* { dg-final { scan-assembler-times "sbcs\t" 8 } } */ +/* { dg-final { scan-assembler-times "ccmp\t" 2 } } */ +/* { dg-final { scan-assembler-times "cset\tw0" 10 } } */ + +int ltu(unsigned __int128 a, unsigned __int128 b) +{ + return a < b; +} + +int gtu(unsigned __int128 a, unsigned __int128 b) +{ + return a > b; +} +int geu(unsigned __int128 a, unsigned __int128 b) +{ + return a >= b; +} +int leu(unsigned __int128 a, unsigned __int128 b) +{ + return a <= b; +} +int eq(unsigned __int128 a, unsigned __int128 b) +{ + return a == b; +} +int ne(unsigned __int128 a, unsigned __int128 b) +{ + return a != b; +} +int lt(__int128 a, __int128 b) +{ + return a < b; +} +int gt(__int128 a, __int128 b) +{ + return a > b; +} +int ge(__int128 a, __int128 b) +{ + return a >= b; +} +int le(__int128 a, __int128 b) +{ + return a <= b; +} + + diff --git a/gcc/testsuite/gcc.target/aarch64/pr116509-2.c b/gcc/testsuite/gcc.target/aarch64/pr116509-2.c new file mode 100644 index 00000000000..aa0dcdf8b07 --- /dev/null +++ b/gcc/testsuite/gcc.target/aarch64/pr116509-2.c @@ -0,0 +1,75 @@ +/* { dg-do compile { target int128 } } */ +/* { dg-options { "-O2" } } */ + +/* PR target/116509. */ +/* 128-bit conditional moves should be handled by a pair of csel + rather than by branching. */ + +/* { dg-final { scan-assembler-not "b\\." } } */ +/* { dg-final { scan-assembler-times "csel\tx0" 10 } } */ +/* { dg-final { scan-assembler-times "csel\tx1" 10 } } */ + +unsigned __int128 +ltu (unsigned __int128 a, unsigned __int128 b, + unsigned __int128 c, unsigned __int128 d) +{ + return a < b ? c : d; +} + +unsigned __int128 +gtu (unsigned __int128 a, unsigned __int128 b, + unsigned __int128 c, unsigned __int128 d) +{ + return a > b ? c : d; +} + +unsigned __int128 +geu (unsigned __int128 a, unsigned __int128 b, + unsigned __int128 c, unsigned __int128 d) +{ + return a >= b ? c : d; +} + +unsigned __int128 +leu (unsigned __int128 a, unsigned __int128 b, + unsigned __int128 c, unsigned __int128 d) +{ + return a <= b ? c : d; +} + +unsigned __int128 +eq (unsigned __int128 a, unsigned __int128 b, + unsigned __int128 c, unsigned __int128 d) +{ + return a == b ? c : d; +} + +unsigned __int128 +ne (unsigned __int128 a, unsigned __int128 b, + unsigned __int128 c, unsigned __int128 d) +{ + return a != b ? c : d; +} + +unsigned __int128 +lt (__int128 a, __int128 b, __int128 c, __int128 d) +{ + return a < b ? c : d; +} +unsigned __int128 +gt (__int128 a, __int128 b, __int128 c, __int128 d) +{ + return a > b ? c : d; +} + +unsigned __int128 +ge(__int128 a, __int128 b, __int128 c, __int128 d) +{ + return a >= b ? c : d; +} + +unsigned __int128 +le(__int128 a, __int128 b, __int128 c, __int128 d) +{ + return a <= b ? c : d; +} diff --git a/gcc/testsuite/gcc.target/aarch64/pr116509-3.c b/gcc/testsuite/gcc.target/aarch64/pr116509-3.c new file mode 100644 index 00000000000..ffbe239cdc5 --- /dev/null +++ b/gcc/testsuite/gcc.target/aarch64/pr116509-3.c @@ -0,0 +1,110 @@ +/* { dg-do compile { target int128 } } */ +/* { dg-options { "-O2" } } */ +/* { dg-final { check-function-bodies "**" "" "" } } */ + +/* PR target/116509. */ +/* 128-bit conditional branches should be handled with cmp + sbcs/ccmp. */ + +int f(void); +int g(void); + +/* +** ltu: +** cmp x[02], x[02] +** sbcs x[0-9]+, x[13], x[13] +** ... +*/ +int ltu(unsigned __int128 a, unsigned __int128 b) +{ + return a < b ? f () : g(); +} +/* +** gtu: +** cmp x[02], x[02] +** sbcs x[0-9]+, x[13], x[13] +** ... +*/ +int gtu(unsigned __int128 a, unsigned __int128 b) +{ + return a > b ? f () : g(); +} +/* +** geu: +** cmp x[02], x[02] +** sbcs x[0-9]+, x[13], x[13] +** ... +*/ +int geu(unsigned __int128 a, unsigned __int128 b) +{ + return a >= b ? f () : g(); +} +/* +** leu: +** cmp x[02], x[02] +** sbcs x[0-9]+, x[13], x[13] +** ... +*/ +int leu(unsigned __int128 a, unsigned __int128 b) +{ + return a <= b ? f () : g(); +} +/* +** eq: +** cmp x[02], x[02] +** ccmp x[13], x[13], 0, eq +** ... +*/ +int eq(unsigned __int128 a, unsigned __int128 b) +{ + return a == b ? f () : g(); +} +/* +** ne: +** cmp x[02], x[02] +** ccmp x[13], x[13], 0, eq +** ... +*/ +int ne(unsigned __int128 a, unsigned __int128 b) +{ + return a != b ? f () : g(); +} +/* +** lt: +** cmp x[02], x[02] +** sbcs x[0-9]+, x[13], x[13] +** ... +*/ +int lt(__int128 a, __int128 b) +{ + return a < b ? f () : g(); +} +/* +** gt: +** cmp x[02], x[02] +** sbcs x[0-9]+, x[13], x[13] +** ... +*/ +int gt(__int128 a, __int128 b) +{ + return a > b ? f () : g(); +} +/* +** ge: +** cmp x[02], x[02] +** sbcs x[0-9]+, x[13], x[13] +** ... +*/ +int ge(__int128 a, __int128 b) +{ + return a >= b ? f () : g(); +} +/* +** le: +** cmp x[02], x[02] +** sbcs x[0-9]+, x[13], x[13] +** ... +*/ +int le(__int128 a, __int128 b) +{ + return a <= b ? f () : g(); +}
