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 normal > > I 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 :) > [...] >>> @@ -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. > 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. Things should be simpler if you do do the split in cstore<mode>4. 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.c >>> new 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(); >>> +}
