On 19/07/18 11:03, Jackson Woodruff wrote: > Hi Richard, > > > On 07/12/2018 05:35 PM, Richard Earnshaw (lists) wrote: >> On 11/07/18 17:48, Jackson Woodruff wrote: >>> Hi Sudi, >>> >>> On 07/10/2018 02:29 PM, Sudakshina Das wrote: >>>> Hi Jackson >>>> >>>> >>>> On Tuesday 10 July 2018 09:37 AM, Jackson Woodruff wrote: >>>>> Hi all, >>>>> >>>>> This patch resolves PR86014. It does so by noticing that the last >>>>> load may clobber the address register without issue (regardless of >>>>> where it exists in the final ldp/stp sequence). That check has been >>>>> changed so that the last register may be clobbered and the testcase >>>>> (gcc.target/aarch64/ldp_stp_10.c) now passes. >>>>> >>>>> Bootstrap and regtest OK. >>>>> >>>>> OK for trunk? >>>>> >>>>> Jackson >>>>> >>>>> Changelog: >>>>> >>>>> gcc/ >>>>> >>>>> 2018-06-25 Jackson Woodruff <jackson.woodr...@arm.com> >>>>> >>>>> PR target/86014 >>>>> * config/aarch64/aarch64.c >>>>> (aarch64_operands_adjust_ok_for_ldpstp): >>>>> Remove address clobber check on last register. >>>>> >>>> This looks good to me but you will need a maintainer to approve it. >>>> The only >>>> thing I would add is that if you could move the comment on top of the >>>> for loop >>>> to this patch. That is, keep the original >>>> /* Check if the addresses are clobbered by load. */ >>>> in your [1/2] and make the comment change in [2/2]. >>> Thanks, change made. OK for trunk? >>> >>> Thanks, >>> >>> Jackson >>> >>> pr86014.patch >>> >>> >>> diff --git a/gcc/config/aarch64/aarch64.c b/gcc/config/aarch64/aarch64.c >>> index >>> da44b33b2bc12f9aa2122cf5194e244437fb31a5..8a027974e9772cacf5f5cb8ec61e8ef62187e879 >>> 100644 >>> --- a/gcc/config/aarch64/aarch64.c >>> +++ b/gcc/config/aarch64/aarch64.c >>> @@ -17071,9 +17071,10 @@ aarch64_operands_adjust_ok_for_ldpstp (rtx >>> *operands, bool load, >>> return false; >>> } >>> - /* Check if addresses are clobbered by load. */ >>> + /* Only the last register in the order in which they occur >>> + may be clobbered by the load. */ >>> if (load) >>> - for (int i = 0; i < num_instructions; i++) >>> + for (int i = 0; i < num_instructions - 1; i++) >>> if (reg_mentioned_p (reg[i], mem[i])) >>> return false; >>> >> Can we have a new test for this? > I've added ldp_stp_13.c that tests for this. >> >> Also, if rclass (which you calculate later) is FP_REGS, then the test is >> redundant since mems can never use FP registers as a base register. > > Yes, makes sense. I've flipped the logic around so that the rclass is > calculated first and is then used to avoid the base register check if > it is not GENERAL_REGS. > > Re-bootstrapped and regtested. > > Is this OK for trunk? >
OK. R. > Thanks, > > Jackson > >> >> R. > > > clobber.patch > > > diff --git a/gcc/config/aarch64/aarch64.c b/gcc/config/aarch64/aarch64.c > index > 1369704da3ed8094c0d4612643794e6392dce05a..3dd891ebd00f24ffa4187f0125b306a3c6671bef > 100644 > --- a/gcc/config/aarch64/aarch64.c > +++ b/gcc/config/aarch64/aarch64.c > @@ -17084,9 +17084,26 @@ aarch64_operands_adjust_ok_for_ldpstp (rtx > *operands, bool load, > return false; > } > > - /* Check if addresses are clobbered by load. */ > - if (load) > - for (int i = 0; i < num_insns; i++) > + /* Check if the registers are of same class. */ > + rclass = REG_P (reg[0]) && FP_REGNUM_P (REGNO (reg[0])) > + ? FP_REGS : GENERAL_REGS; > + > + for (int i = 1; i < num_insns; i++) > + if (REG_P (reg[i]) && FP_REGNUM_P (REGNO (reg[i]))) > + { > + if (rclass != FP_REGS) > + return false; > + } > + else > + { > + if (rclass != GENERAL_REGS) > + return false; > + } > + > + /* Only the last register in the order in which they occur > + may be clobbered by the load. */ > + if (rclass == GENERAL_REGS && load) > + for (int i = 0; i < num_insns - 1; i++) > if (reg_mentioned_p (reg[i], mem[i])) > return false; > > @@ -17126,22 +17143,6 @@ aarch64_operands_adjust_ok_for_ldpstp (rtx > *operands, bool load, > && MEM_ALIGN (mem[0]) < 8 * BITS_PER_UNIT) > return false; > > - /* Check if the registers are of same class. */ > - rclass = REG_P (reg[0]) && FP_REGNUM_P (REGNO (reg[0])) > - ? FP_REGS : GENERAL_REGS; > - > - for (int i = 1; i < num_insns; i++) > - if (REG_P (reg[i]) && FP_REGNUM_P (REGNO (reg[i]))) > - { > - if (rclass != FP_REGS) > - return false; > - } > - else > - { > - if (rclass != GENERAL_REGS) > - return false; > - } > - > return true; > } > > diff --git a/gcc/testsuite/gcc.target/aarch64/ldp_stp_13.c > b/gcc/testsuite/gcc.target/aarch64/ldp_stp_13.c > new file mode 100644 > index > 0000000000000000000000000000000000000000..9cc3942f153773e8ffe9bcaf07f6b32dc0d5f95e > --- /dev/null > +++ b/gcc/testsuite/gcc.target/aarch64/ldp_stp_13.c > @@ -0,0 +1,18 @@ > +/* { dg-do compile } */ > +/* { dg-options "-O2 -mabi=ilp32" } */ > + > +long long > +load_long (long long int *arr) > +{ > + return arr[400] << 1 + arr[401] << 1 + arr[403] << 1 + arr[404] << 1; > +} > + > +/* { dg-final { scan-assembler-times "ldp\tx\[0-9\]+, x\[0-9\]+, " 2 } } */ > + > +int > +load (int *arr) > +{ > + return arr[527] << 1 + arr[400] << 1 + arr[401] << 1 + arr[528] << 1; > +} > + > +/* { dg-final { scan-assembler-times "ldp\tw\[0-9\]+, w\[0-9\]+, " 2 } } */ >