RE: [PATCH][Aarch64] v2: Arithmetic overflow addv patterns [Patch 2/4]

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

All requested changes made:

- label_ref added as operand 3
- more meaningful names given to variables

Okay for trunk?
-----Original Message-----
From: James Greenhalgh <james.greenhalgh@arm.com> 
Sent: Thursday, June 7, 2018 5:29 PM
To: Michael Collison <Michael.Collison@arm.com>
Cc: GCC Patches <gcc-patches@gcc.gnu.org>; nd <nd@arm.com>
Subject: Re: [PATCH][Aarch64] v2: Arithmetic overflow addv patterns [Patch 2/4]

On Wed, Jun 06, 2018 at 12:16:22PM -0500, Michael Collison wrote:
> This is a respin of a AArch64 patch that adds support for builtin arithmetic overflow operations. This update separates the patch into multiple pieces and addresses comments made by Richard Earnshaw here:
> 
> https://gcc.gnu.org/ml/gcc-patches/2017-07/msg00249.html
> 
> Original patch and motivation for patch here:
> 
> https://gcc.gnu.org/ml/gcc-patches/2017-05/msg01512.html
> 
> This patch contains new patterns for addv<mode> overflow patterns.
> 
> Bootstrapped and tested on aarch64-linux-gnu. Okay for trunk?

> +(define_expand "addv<mode>4"
> +  [(match_operand:GPI 0 "register_operand")
> +   (match_operand:GPI 1 "register_operand")
> +   (match_operand:GPI 2 "register_operand")
> +   (match_operand 3 "")]
> +  ""

It won't be validated; but I'd prefer us to add the constraint on the label so this code is self-documenting. It would have saved me a trip to the manual to understand operand 3.

>  (define_expand "addti3"
>    [(set (match_operand:TI 0 "register_operand" "")
>  	(plus:TI (match_operand:TI 1 "register_operand" "")
> -		 (match_operand:TI 2 "register_operand" "")))]
> +		 (match_operand:TI 2 "aarch64_reg_or_imm" "")))]
>    ""
>  {
> -  rtx low = gen_reg_rtx (DImode);
> -  emit_insn (gen_adddi3_compareC (low, gen_lowpart (DImode, operands[1]),
> -				  gen_lowpart (DImode, operands[2])));
> +  rtx l0,l1,l2,h0,h1,h2;

Let's give these slightly meaningful names please. dest_high, dest_low, op1_high, etc.

Other than these two comments, I think this is OK.

There are some subtleties in here though that I've probably missed, so I wouldn't say no to a second pair of eyes.

Thanks,
James


> 
> 
> 2018-05-31  Michael Collison  <michael.collison@arm.com>
> 	    Richard Henderson <rth@redhat.com>
> 
> 	* config/aarch64/aarch64.md: (addv<GPI>4, uaddv<GPI>4): New.
> 	(addti3): Create simpler code if low part is already known to be 0.
> 	(addvti4, uaddvti4): New.
> 	(*add<GPI>3_compareC_cconly_imm): New.
> 	(*add<GPI>3_compareC_cconly): New.
> 	(*add<GPI>3_compareC_imm): New.
> 	(*add<GPI>3_compareC): Rename from add<GPI>3_compare1; do not
> 	handle constants within this pattern..
> 	(*add<GPI>3_compareV_cconly_imm): New.
> 	(*add<GPI>3_compareV_cconly): New.
> 	(*add<GPI>3_compareV_imm): New.
> 	(add<GPI>3_compareV): New.
> 	(add<GPI>3_carryinC, add<GPI>3_carryinV): New.
> 	(*add<GPI>3_carryinC_zero, *add<GPI>3_carryinV_zero): New.
> 	(*add<GPI>3_carryinC, *add<GPI>3_carryinV): New.
> 	((*add<GPI>3_compareC_cconly_imm): Replace 'ne' operator
> 	with 'comparison' operator.
> 	(*add<GPI>3_compareV_cconly_imm): Ditto.
> 	(*add<GPI>3_compareV_cconly): Ditto.
> 	(*add<GPI>3_compareV_imm): Ditto.
> 	(add<GPI>3_compareV): Ditto.
> 	(add<mode>3_carryinC): Ditto.
> 	(*add<mode>3_carryinC_zero): Ditto.
> 	(*add<mode>3_carryinC): Ditto.
> 	(add<mode>3_carryinV): Ditto.
> 	(*add<mode>3_carryinV_zero): Ditto.
> 	(*add<mode>3_carryinV): Ditto.

Attachment: gnutools-6308-pt2.patch
Description: gnutools-6308-pt2.patch