On Wed, Jun 06, 2018 at 12:14:03PM -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 primarily contains common functions in aarch64.c for generating
> TImode scratch registers,
> and common rtl functions utilized by the overflow patterns in aarch64.md. In
> addition a new mode representing overflow CC_Vmode is introduced.
>
> Bootstrapped and tested on aarch64-linux-gnu. Okay for trunk?
Normally it is preferred that each patch in a series stands independent of
the others. So if I apply just 1/4 I should get a working toolchain. You
have some dependencies here between 1/4 and 3/4.
Rather than ask you to rework these patches, I think I'll instead ask you to
squash them all to a single commit after we're done with review. That will
save you some rebase work and maintain the property that trunk can be built
at most revisions.
> (aarch64_add_128bit_scratch_regs): Declare
> (aarch64_subv_128bit_scratch_regs): Declare.
Why use 128bit in the function name rather than
call it aarch64_subvti_scratch_regs ?
> @@ -16337,6 +16353,131 @@ aarch64_split_dimode_const_store (rtx dst, rtx src)
> return true;
> }
>
> +/* Generate RTL for a conditional branch with rtx comparison CODE in
> + mode CC_MODE. The destination of the unlikely conditional branch
> + is LABEL_REF. */
> +
> +void
> +aarch64_gen_unlikely_cbranch (enum rtx_code code, machine_mode cc_mode,
> + rtx label_ref)
> +{
> + rtx x;
> + x = gen_rtx_fmt_ee (code, VOIDmode,
> + gen_rtx_REG (cc_mode, CC_REGNUM),
> + const0_rtx);
> +
> + x = gen_rtx_IF_THEN_ELSE (VOIDmode, x,
> + gen_rtx_LABEL_REF (VOIDmode, label_ref),
> + pc_rtx);
> + aarch64_emit_unlikely_jump (gen_rtx_SET (pc_rtx, x));
> +}
> +
I'm a bit surprised this is AArh64 specific and there are no helper functions
to get you here. Not that it should block the patch;l but if we can reuse
something I'd prefer we did.
> +void
> +aarch64_expand_subvti (rtx op0, rtx low_dest, rtx low_in1,
> + rtx low_in2, rtx high_dest, rtx high_in1,
> + rtx high_in2)
> +{
> + if (low_in2 == const0_rtx)
> + {
> + low_dest = low_in1;
> + emit_insn (gen_subdi3_compare1 (high_dest, high_in1,
> + force_reg (DImode, high_in2)));
> + }
> + else
> + {
> + if (CONST_INT_P (low_in2))
> + {
> + low_in2 = force_reg (DImode, GEN_INT (-UINTVAL (low_in2)));
> + high_in2 = force_reg (DImode, high_in2);
> + emit_insn (gen_adddi3_compareC (low_dest, low_in1, low_in2));
> + }
> + else
> + emit_insn (gen_subdi3_compare1 (low_dest, low_in1, low_in2));
> + emit_insn (gen_subdi3_carryinCV (high_dest,
> + force_reg (DImode, high_in1),
> + high_in2));
This is where we'd break the build. gen_subdi3_carryinCV isn't defined
until 3/4.
The above points are minor.
This patch is OK with them cleaned up, once I've reviewed the other 3 parts
to this series.
James
>
> 2018-05-31 Michael Collison <[email protected]>
> Richard Henderson <[email protected]>
>
> * config/aarch64/aarch64-modes.def (CC_V): New.
> * config/aarch64/aarch64-protos.h
> (aarch64_add_128bit_scratch_regs): Declare
> (aarch64_subv_128bit_scratch_regs): Declare.
> (aarch64_expand_subvti): Declare.
> (aarch64_gen_unlikely_cbranch): Declare
> * config/aarch64/aarch64.c (aarch64_select_cc_mode): Test
> for signed overflow using CC_Vmode.
> (aarch64_get_condition_code_1): Handle CC_Vmode.
> (aarch64_gen_unlikely_cbranch): New function.
> (aarch64_add_128bit_scratch_regs): New function.
> (aarch64_subv_128bit_scratch_regs): New function.
> (aarch64_expand_subvti): New function.
