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.colli...@arm.com>
> Richard Henderson <r...@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.
