> -----Original Message----- > From: Richard Sandiford <richard.sandif...@arm.com> > Sent: 30 April 2020 15:13 > To: Alex Coplan <alex.cop...@arm.com> > Cc: gcc-patches@gcc.gnu.org; Richard Earnshaw <richard.earns...@arm.com>; > Marcus Shawcroft <marcus.shawcr...@arm.com>; Kyrylo Tkachov > <kyrylo.tkac...@arm.com>; nd <n...@arm.com> > Subject: Re: [PATCH] aarch64: prefer using csinv, csneg in zero extend > contexts > > Yeah, I was hoping for something like... > > > Indeed, clang generates a MVN + CSEL sequence where the CSEL operates on the > > 64-bit registers: > > > > f: > > mvn w8, w2 > > cmp w0, #0 > > csel x0, x8, x1, eq > > ret > > ...this rather than the 4-insn (+ret) sequence that we currently > generate. So it would have been a define_insn_and_split that handles > the zero case directly but splits into the "optimal" two-instruction > sequence for registers. > > But I guess the underlying problem is instead that we don't have > a pattern for (zero_extend:DI (not:SI ...)). Adding that would > definitely be a better fix.
Yes. I sent a patch for this very fix which Kyrill is going to commit once stage 1 opens: https://gcc.gnu.org/pipermail/gcc-patches/2020-April/544365.html I tried compiling this function with that patch applied and we get: f: cmp w0, 0 mvn w2, w2 csel x0, x2, x1, eq ret which is good. > ChangeLog trivia, but these last two lines should only be indented by a tab. Ah, thanks. Noted for next time. > > Hopefully one day we'll finally ditch this format and stop having to > quibble over such minor formatting stuff... That would be good! > > > diff --git a/gcc/config/aarch64/aarch64.md b/gcc/config/aarch64/aarch64.md > > index c7c4d1dd519..37d651724ad 100644 > > --- a/gcc/config/aarch64/aarch64.md > > +++ b/gcc/config/aarch64/aarch64.md > > @@ -4390,6 +4390,45 @@ > > [(set_attr "type" "csel")] > > ) > > > > +(define_insn "*csinv3_uxtw_insn1" > > + [(set (match_operand:DI 0 "register_operand" "=r") > > + (if_then_else:DI > > + (match_operand 1 "aarch64_comparison_operation" "") > > + (zero_extend:DI > > + (match_operand:SI 2 "register_operand" "r")) > > + (zero_extend:DI > > + (NEG_NOT:SI (match_operand:SI 3 "register_operand" "r")))))] > > + "" > > + "cs<neg_not_cs>\\t%w0, %w2, %w3, %m1" > > + [(set_attr "type" "csel")] > > +) > > + > > +(define_insn "*csinv3_uxtw_insn2" > > + [(set (match_operand:DI 0 "register_operand" "=r") > > + (if_then_else:DI > > + (match_operand 1 "aarch64_comparison_operation" "") > > + (zero_extend:DI > > + (NEG_NOT:SI (match_operand:SI 2 "register_operand" "r"))) > > + (zero_extend:DI > > + (match_operand:SI 3 "register_operand" "r"))))] > > + "" > > + "cs<neg_not_cs>\\t%w0, %w3, %w2, %M1" > > + [(set_attr "type" "csel")] > > +) > > + > > +(define_insn "*csinv3_uxtw_insn3" > > + [(set (match_operand:DI 0 "register_operand" "=r") > > + (if_then_else:DI > > + (match_operand 1 "aarch64_comparison_operation" "") > > + (zero_extend:DI > > + (NEG_NOT:SI (match_operand:SI 2 "register_operand" "r"))) > > + (const_int 0)))] > > + "" > > + "cs<neg_not_cs>\\t%w0, wzr, %w2, %M1" > > + [(set_attr "type" "csel")] > > +) > > + > > + > > Usually there's just one blank line between patterns, even if the > patterns aren't naturally grouped. Ok, good to know. > > No need to repost just for that. I'll push with those changes once > stage 1 opens. Great! Thanks, Alex
