On 10/13/26 1:29 PM, Richard Sandiford wrote:
David Faust <[email protected]> writes: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.No worries.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 normalI think you mean that the tests should be named for the insn rather than the PR, right?No, sorry. It was a half-complete thought that I forgot to delete before sending. So please ignore :)
Ah ok :)
[...]@@ -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.Did those examples include cases where the destination is a SUBREG though? If the destination is a plain REG then everything's ok, since the above gen_reg_rtx wouldn't be used.
Evidently not...
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.I think the code essentially leaves dst unused in the !REG_P case, and leaves the old operands[0] undefined. But of course, it's still possible for a lucky allocation to assign the same hard register to both.
...and you're totally right. It can also ICE, because the gen_reg_rtx may give back e.g. (reg:TI 32 v0) which causes the gen_lowpart/highpart to fail.
Things should be simpler if you do do the split in cstore<mode>4.
Right, thanks!
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.mdb/gcc/config/aarch64/iterators.mdindex 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-bitmodes)(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 thesemodes.(define_mode_iterator GPI_I16 [(HI "TARGET_FP_F16INST") SI DI])diff --git a/gcc/testsuite/gcc.target/aarch64/pr116509-1.cb/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.cb/gcc/testsuite/gcc.target/aarch64/pr116509-2.cnew 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.cb/gcc/testsuite/gcc.target/aarch64/pr116509-3.cnew 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(); +}
