This is the mail archive of the gcc-patches@gcc.gnu.org mailing list for the GCC project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: [PATCH] [Aarch64] Variable shift count truncation issues


Resending for the list, as the last copy got bounced.

Thanks,
James

----- Forwarded message from James Greenhalgh <james.greenhalgh@arm.com> -----

Date: Thu, 22 Jun 2017 11:16:38 +0100
From: James Greenhalgh <james.greenhalgh@arm.com>
To: Michael Collison <Michael.Collison@arm.com>, Wilco Dijkstra <Wilco.Dijkstra@arm.com>, Christophe Lyon <christophe.lyon@linaro.org>, GCC Patches <gcc-patches@gcc.gnu.org>, nd <nd@arm.com>, richard.sandiford@linaro.org
Subject: Re: [PATCH] [Aarch64] Variable shift count truncation issues
User-Agent: Mutt/1.5.21 (2010-09-15)

On Wed, Jun 21, 2017 at 04:42:00PM +0100, Richard Sandiford wrote:
> Michael Collison <Michael.Collison@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.tkachov@arm.com>
> > 	    Michael Collison <michael.collison@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.tkachov@arm.com>
> > 	    Michael Collison <michael.collison@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


----- End forwarded message -----


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]