[AArch64] Generate load-pairs when the last load clobbers the address register [2/2]

[Richard Earnshaw (lists)]

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.woodruff@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 } } */
>