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

Message ID 14c4dfab-ef38-5ae5-be40-2f7ea433642d@arm.com
State New
Headers show
Series
  • [AArch64] Generate load-pairs when the last load clobbers the address register [2/2]
Related show

Commit Message

Jackson Woodruff July 10, 2018, 8:37 a.m.
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.

Comments

Sudakshina Das July 10, 2018, 1:29 p.m. | #1
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].

diff --git a/gcc/config/aarch64/aarch64.c b/gcc/config/aarch64/aarch64.c
index d0e9b2d464183eecc8cc7639ca3e981d2ff243ba..feffe8ebdbd4efd0ffc09834547767ceec46f4e4 100644
--- a/gcc/config/aarch64/aarch64.c
+++ b/gcc/config/aarch64/aarch64.c
@@ -17074,7 +17074,7 @@ aarch64_operands_adjust_ok_for_ldpstp (rtx *operands, bool 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;


Thanks
Sudi
Jackson Woodruff July 11, 2018, 4:48 p.m. | #2
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
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;
Sudakshina Das July 12, 2018, 10:09 a.m. | #3
Hi Jackson

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?
> 

Looks good to me but you will need approval from
a maintainer to commit it!

Thanks
Sudi

> Thanks,
> 
> Jackson
Richard Earnshaw (lists) July 12, 2018, 4:35 p.m. | #4
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?

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.

R.
Jackson Woodruff July 19, 2018, 10:03 a.m. | #5
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?

Thanks,

Jackson

>
> R.
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 } } */
Richard Earnshaw (lists) Aug. 1, 2018, 3:06 p.m. | #6
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 } } */
>

Patch

diff --git a/gcc/config/aarch64/aarch64.c b/gcc/config/aarch64/aarch64.c
index d0e9b2d464183eecc8cc7639ca3e981d2ff243ba..feffe8ebdbd4efd0ffc09834547767ceec46f4e4 100644
--- a/gcc/config/aarch64/aarch64.c
+++ b/gcc/config/aarch64/aarch64.c
@@ -17074,7 +17074,7 @@  aarch64_operands_adjust_ok_for_ldpstp (rtx *operands, bool 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;