On 11/04/2019 16:21, Steve Ellcey wrote: > On Thu, 2019-04-11 at 14:58 +0000, Steve Ellcey wrote: >> >>> You've removed the ..._noshift_alt variant. That wasn't my >>> intention, >>> so perhaps you misunderstood what I was trying to say. >>> >>> The two versions are both needed, since the register tie is not >>> orthogonal to the constants used in the masks and canonicalization >>> will >>> not generate a consistent ordering of the operands. >> >> I started doing this and then convinced myself (perhaps incorrectly) >> that I didn't need the alt version. Operands 1 and 3 are registers >> and Operands 2 and 4 are constants, so the only difference is in the >> call to aarch64_masks_and_shift_for_bfi_p. Given that argument 2 to >> this call is 0 this call should be the equivelent of ((x & MASK1) | >> (y >> & MASK2)) and that should mean that: >> >> aarch64_masks_and_shift_for_bfi_p(X,0,Y) == >> aarch64_masks_and_shift_for_bfi_p(Y,0,X) >> >> >> Maybe I am wrong about that? I will do some expirements. My testing >> did not find any cases in the testsuite where not having the _alt >> version resulted in a bfi not being generated. > > OK, I think I see where my idea that I didn't need both versions is > wrong. There is an extra check on the final argument that is not done > on the initial argument. Here is the patch that I am testing, I think > it is what you have in mind. It looks wierd having arguments 3 and 4 > before 1 and 2, I think that is why I had it differently in my original > patch but if you prefer this version, that is fine with me. > > OK to check in once my build/test is finished? > > > > 2018-04-11 Steve Ellcey <sell...@marvell.com> > > PR rtl-optimization/87763 > * config/aarch64/aarch64.md (*aarch64_bfi<GPI:mode>4_noshift): > New Instruction. > > > diff --git a/gcc/config/aarch64/aarch64.md > b/gcc/config/aarch64/aarch64.md > index e0df975..eac688a 100644 > --- a/gcc/config/aarch64/aarch64.md > +++ b/gcc/config/aarch64/aarch64.md > @@ -5565,7 +5565,8 @@ > ) > > ;; Like *aarch64_bfi<GPI:mode>5_shift but with no shifting, we are just > -;; copying the least significant bits of OP3 to OP0. > +;; copying the least significant bits of OP3 to OP0. We need two versions > +;; of the instruction to handle different checks on the constant values. > > (define_insn "*aarch64_bfi<GPI:mode>4_noshift" > [(set (match_operand:GPI 0 "register_operand" "=r") > @@ -5579,6 +5580,18 @@ > [(set_attr "type" "bfm")] > ) > > +(define_insn "*aarch64_bfi<GPI:mode>4_noshift"
Please add _alt at the end, to distinguish from the insn above. Otherwise OK. R. > + [(set (match_operand:GPI 0 "register_operand" "=r") > + (ior:GPI (and:GPI (match_operand:GPI 3 "register_operand" "0") > + (match_operand:GPI 4 "const_int_operand" "n")) > + (and:GPI (match_operand:GPI 1 "register_operand" "r") > + (match_operand:GPI 2 "const_int_operand" "n"))))] > + "aarch64_masks_and_shift_for_bfi_p (<MODE>mode, UINTVAL (operands[2]), 0, > + UINTVAL (operands[4]))" > + "bfi\t%<GPI:w>0, %<GPI:w>3, 0, %P4" > + [(set_attr "type" "bfm")] > +) > + > (define_insn "*extr_insv_lower_reg<mode>" > [(set (zero_extract:GPI (match_operand:GPI 0 "register_operand" "+r") > (match_operand 1 "const_int_operand" "n") >
