Fixed the "nitpick" issues pointed out by James. Okay for trunk? 2017-05-22 Kyrylo Tkachov <kyrylo.tkac...@arm.com> Michael Collison <michael.colli...@arm.com>
PR target/70119 * config/aarch64/aarch64.md (*aarch64_<optab>_reg_<mode>3_mask1): New pattern. (*aarch64_reg_<mode>3_neg_mask2): New pattern. (*aarch64_reg_<mode>3_minus_mask): New pattern. (*aarch64_<optab>_reg_di3_mask2): New pattern. * config/aarch64/aarch64.c (aarch64_rtx_costs): Account for cost of shift when the shift amount is masked with constant equal to the size of the mode. * config/aarch64/predicates.md (subreg_lowpart_operator): New predicate. 2016-05-22 Kyrylo Tkachov <kyrylo.tkac...@arm.com> Michael Collison <michael.colli...@arm.com> PR target/70119 * gcc.target/aarch64/var_shift_mask_1.c: New test. -----Original Message----- From: James Greenhalgh [mailto:james.greenha...@arm.com] Sent: Thursday, June 22, 2017 3:17 AM To: Michael Collison <michael.colli...@arm.com>; Wilco Dijkstra <wilco.dijks...@arm.com>; Christophe Lyon <christophe.l...@linaro.org>; GCC Patches <gcc-patches@gcc.gnu.org>; nd <n...@arm.com>; richard.sandif...@linaro.org Subject: Re: [PATCH] [Aarch64] Variable shift count truncation issues On Wed, Jun 21, 2017 at 04:42:00PM +0100, Richard Sandiford wrote: > Michael Collison <michael.colli...@arm.com> writes: > > Updated the patch per Richard's suggestions to allow scheduling of > > instructions before reload. > > Thanks, this looks good to me FWIW, but obviously I can't approve it. Thanks for the review Richard, that gives me good confidence in this patch. I have a few comments below, which are closer to nitpicking than structural issues with the patch. With those fixed, this is OK to commit. > > 2017-05-22 Kyrylo Tkachov <kyrylo.tkac...@arm.com> > > Michael Collison <michael.colli...@arm.com> With the work you've done, you can probably place yourself first on the ChangeLog now ;) > > > > PR target/70119 > > * config/aarch64/aarch64.md (*aarch64_<optab>_reg_<mode>3_mask1): > > New pattern. > > (*aarch64_reg_<mode>3_neg_mask2): New pattern. > > (*aarch64_reg_<mode>3_minus_mask): New pattern. > > (*aarch64_<optab>_reg_di3_mask2): New pattern. > > * config/aarch64/aarch64.c (aarch64_rtx_costs): Account for cost > > of shift when the shift amount is masked with constant equal to > > the size of the mode. > > * config/aarch64/predicates.md (subreg_lowpart_operator): New > > predicate. > > > > > > 2016-05-22 Kyrylo Tkachov <kyrylo.tkac...@arm.com> > > Michael Collison <michael.colli...@arm.com> > > > > PR target/70119 > > * gcc.target/aarch64/var_shift_mask_1.c: New test. > > diff --git a/gcc/config/aarch64/aarch64.md > > b/gcc/config/aarch64/aarch64.md index d89df66..ff5720c 100644 > > --- a/gcc/config/aarch64/aarch64.md > > +++ b/gcc/config/aarch64/aarch64.md > > @@ -3942,6 +3942,97 @@ > > } > > ) > > > > +;; When the LSL, LSR, ASR, ROR instructions operate on all register > > +arguments ;; they truncate the shift/rotate amount by the size of > > +the registers they ;; operate on: 32 for W-regs, 63 for X-regs. > > +This allows us to optimise away Is this "63" a typo? Should it be 64? > > +;; such redundant masking instructions. GCC can do that > > +automatically when ;; SHIFT_COUNT_TRUNCATED is true, but we can't > > +enable it for TARGET_SIMD ;; because some of the SISD shift alternatives > > don't perform this truncations. > > +;; So this pattern exists to catch such cases. > > + > > +(define_insn "*aarch64_<optab>_reg_<mode>3_mask1" > > + [(set (match_operand:GPI 0 "register_operand" "=r") > > + (SHIFT:GPI > > + (match_operand:GPI 1 "register_operand" "r") > > + (match_operator 4 "subreg_lowpart_operator" > > + [(and:GPI (match_operand:GPI 2 "register_operand" "r") > > + (match_operand 3 "const_int_operand" "n"))])))] > > + "(~INTVAL (operands[3]) & (GET_MODE_BITSIZE (<MODE>mode)-1)) == 0" Spaces around "-" > > + "<shift>\t%<w>0, %<w>1, %<w>2" > > + [(set_attr "type" "shift_reg")] > > +) > > + > > +(define_insn_and_split "*aarch64_reg_<mode>3_neg_mask2" > > + [(set (match_operand:GPI 0 "register_operand" "=&r") > > + (SHIFT:GPI > > + (match_operand:GPI 1 "register_operand" "r") > > + (match_operator 4 "subreg_lowpart_operator" > > + [(neg:SI (and:SI (match_operand:SI 2 "register_operand" "r") > > + (match_operand 3 "const_int_operand" "n")))])))] > > + "((~INTVAL (operands[3]) & (GET_MODE_BITSIZE (<MODE>mode)-1)) == 0)" > > + "#" > > + "&& 1" I'd prefer "true" to "1" > > + [(const_int 0)] > > + { > > + rtx tmp = (can_create_pseudo_p () ? gen_reg_rtx (SImode) > > + : operands[0]); > > + emit_insn (gen_negsi2 (tmp, operands[2])); > > + > > + rtx and_op = gen_rtx_AND (SImode, tmp, operands[3]); > > + rtx subreg_tmp = gen_rtx_SUBREG (GET_MODE (operands[4]), and_op, > > + SUBREG_BYTE (operands[4])); > > + emit_insn (gen_<optab><mode>3 (operands[0], operands[1], subreg_tmp)); > > + DONE; > > + } > > +) > > + > > +(define_insn_and_split "*aarch64_reg_<mode>3_minus_mask" > > + [(set (match_operand:GPI 0 "register_operand" "=&r") > > + (ashift:GPI > > + (match_operand:GPI 1 "register_operand" "r") > > + (minus:QI (match_operand 2 "const_int_operand" "n") > > + (match_operator 5 "subreg_lowpart_operator" > > + [(and:SI (match_operand:SI 3 "register_operand" "r") > > + (match_operand 4 "const_int_operand" "n"))]))))] > > + "((~INTVAL (operands[4]) & (GET_MODE_BITSIZE (<MODE>mode)-1)) == 0) > > + && INTVAL (operands[2]) == GET_MODE_BITSIZE (<MODE>mode)" > > + "#" > > + "&& 1" Likewise. Thanks, James
pr5546v6.patch
Description: pr5546v6.patch