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 > > 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. > { > 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. > + 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 rtx > diff --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. > + [(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]. 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(); > +}
