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

[Michael Collison <Michael dot Collison at arm dot com>]

Fixed the "nitpick" issues pointed out by James. Okay for trunk?

2017-05-22  Kyrylo Tkachov  <kyrylo.tkachov@arm.com>
	    Michael Collison <michael.collison@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.tkachov@arm.com>
	    Michael Collison <michael.collison@arm.com>

	PR target/70119
	* gcc.target/aarch64/var_shift_mask_1.c: New test.

-----Original Message-----
From: James Greenhalgh [mailto:james.greenhalgh@arm.com] 
Sent: Thursday, June 22, 2017 3:17 AM
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

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

Attachment: pr5546v6.patch
Description: pr5546v6.patch