[reload,tentative,PR,71627] Tweak conditions in find_valid_class_1

Message ID 871szkg2he.fsf@atmel.com
State New
Headers show

Commit Message

Senthil Kumar Selvaraj Oct. 13, 2016, 6:57 a.m.
Bernd Schmidt writes:

> On 09/16/2016 09:02 PM, Senthil Kumar Selvaraj wrote:
>>   Does this make sense? I ran a reg test for the avr target with a
>>   slightly older version of this patch, it did not show any regressions.
>>   If this is the right fix, I'll make sure to run reg tests on x86_64
>>   after backporting to a gcc version where that target used reload.
>
> It's hard to say, and could have different effects on different targets.
> One thing though, at the very least the reg_class_size test would have 
> to be adapted - the idea is to find the largest class, and there's a 
> risk here of ending up with a large class that only has one valid register.

Agreed - I've updated the patch to compute rclass sizes based on regno
availability i.e., only if in_hard_reg_set_p and HARD_REGNO_MODE_OK, and
then use the computed sizes when calculating best_size.

>
> You'll also want to verify this against
>    https://gcc.gnu.org/bugzilla/show_bug.cgi?id=54814

Yes, this patch doesn't break the fix for PR54814. The change to
in_hard_reg_set_p was what fixed that, and that remains unmodified.

Reg tested this on top of trunk@190252 with the in_hard_reg_set_p
backport. x86_64-pc-linux bootstrapped and regtested ok. avr showed
no regressions either.

Ok for trunk?

Regards
Senthil

gcc/ChangeLog:

2016-10-13  Senthil Kumar Selvaraj  <senthil_kumar.selvaraj@atmel.com>

	* reload.c (find_valid_class_1): Allow regclass if atleast one
	regno in class is ok. Compute and use rclass size based on
	actually available regnos for mode in rclass.

gcc/testsuite/ChangeLog:

2016-10-13  Senthil Kumar Selvaraj  <senthil_kumar.selvaraj@atmel.com>
	
	* gcc.target/avr/pr71627.c: New.

Comments

Senthil Kumar Selvaraj Oct. 18, 2016, 9:17 a.m. | #1
Ping!

Regards
Senthil

Senthil Kumar Selvaraj writes:

> Bernd Schmidt writes:
>
>> On 09/16/2016 09:02 PM, Senthil Kumar Selvaraj wrote:
>>>   Does this make sense? I ran a reg test for the avr target with a
>>>   slightly older version of this patch, it did not show any regressions.
>>>   If this is the right fix, I'll make sure to run reg tests on x86_64
>>>   after backporting to a gcc version where that target used reload.
>>
>> It's hard to say, and could have different effects on different targets.
>> One thing though, at the very least the reg_class_size test would have 
>> to be adapted - the idea is to find the largest class, and there's a 
>> risk here of ending up with a large class that only has one valid register.
>
> Agreed - I've updated the patch to compute rclass sizes based on regno
> availability i.e., only if in_hard_reg_set_p and HARD_REGNO_MODE_OK, and
> then use the computed sizes when calculating best_size.
>
>>
>> You'll also want to verify this against
>>    https://gcc.gnu.org/bugzilla/show_bug.cgi?id=54814
>
> Yes, this patch doesn't break the fix for PR54814. The change to
> in_hard_reg_set_p was what fixed that, and that remains unmodified.
>
> Reg tested this on top of trunk@190252 with the in_hard_reg_set_p
> backport. x86_64-pc-linux bootstrapped and regtested ok. avr showed
> no regressions either.
>
> Ok for trunk?
>
> Regards
> Senthil
>
> gcc/ChangeLog:
>
> 2016-10-13  Senthil Kumar Selvaraj  <senthil_kumar.selvaraj@atmel.com>
>
> 	* reload.c (find_valid_class_1): Allow regclass if atleast one
> 	regno in class is ok. Compute and use rclass size based on
> 	actually available regnos for mode in rclass.
>
> gcc/testsuite/ChangeLog:
>
> 2016-10-13  Senthil Kumar Selvaraj  <senthil_kumar.selvaraj@atmel.com>
> 	
> 	* gcc.target/avr/pr71627.c: New.
>
>
> Index: gcc/reload.c
> ===================================================================
> --- gcc/reload.c	(revision 240989)
> +++ gcc/reload.c	(working copy)
> @@ -711,31 +711,36 @@
>    enum reg_class best_class = NO_REGS;
>    unsigned int best_size = 0;
>    int cost;
> +  unsigned int computed_rclass_sizes[N_REG_CLASSES] = { 0 };
>  
>    for (rclass = 1; rclass < N_REG_CLASSES; rclass++)
>      {
> -      int bad = 0;
> -      for (regno = 0; regno < FIRST_PSEUDO_REGISTER && !bad; regno++)
> -	{
> -	  if (in_hard_reg_set_p (reg_class_contents[rclass], mode, regno)
> -	      && !HARD_REGNO_MODE_OK (regno, mode))
> -	    bad = 1;
> -	}
> -      
> -      if (bad)
> -	continue;
> +      int atleast_one_regno_ok = 0;
>  
> +      for (regno = 0; regno < FIRST_PSEUDO_REGISTER; regno++)
> +        {
> +          if (in_hard_reg_set_p (reg_class_contents[rclass], mode, regno))
> +            {
> +              atleast_one_regno_ok = 1;
> +              if (HARD_REGNO_MODE_OK (regno, mode))
> +                computed_rclass_sizes[rclass]++;
> +            }
> +        }
> +
> +      if (!atleast_one_regno_ok)
> +        continue;
> +
>        cost = register_move_cost (outer, (enum reg_class) rclass, dest_class);
>  
> -      if ((reg_class_size[rclass] > best_size
> -	   && (best_cost < 0 || best_cost >= cost))
> -	  || best_cost > cost)
> -	{
> -	  best_class = (enum reg_class) rclass;
> -	  best_size = reg_class_size[rclass];
> -	  best_cost = register_move_cost (outer, (enum reg_class) rclass,
> -					  dest_class);
> -	}
> +      if ((computed_rclass_sizes[rclass] > best_size
> +	    && (best_cost < 0 || best_cost >= cost))
> +	   || best_cost > cost)
> +	 {
> +	   best_class = (enum reg_class) rclass;
> +	   best_size = computed_rclass_sizes[rclass];
> +	   best_cost = register_move_cost (outer, (enum reg_class) rclass,
> +					   dest_class);
> +	 }
>      }
>  
>    gcc_assert (best_size != 0);
>
> Index: gcc/testsuite/gcc.target/avr/pr71627.c
> ===================================================================
> --- gcc/testsuite/gcc.target/avr/pr71627.c      (nonexistent)
> +++ gcc/testsuite/gcc.target/avr/pr71627.c      (working copy)
> @@ -0,0 +1,14 @@
> +/* { dg-do compile } */
> +/* { dg-options "-O1" } */
> +
> +
> +extern volatile __memx const long  a, b, c, d, e, f;
> +extern volatile long result;
> +
> +extern void vfunc (const char*, ...);
> +
> +void foo (void)
> +{
> +       result = a + b + c + d + e + f;
> +       vfunc ("text", a, b, c, d, e, f, result);
> +}
Bernd Schmidt Oct. 18, 2016, 10:34 a.m. | #2
On 10/13/2016 08:57 AM, Senthil Kumar Selvaraj wrote:
>
> 2016-10-13  Senthil Kumar Selvaraj  <senthil_kumar.selvaraj@atmel.com>
>
> 	* reload.c (find_valid_class_1): Allow regclass if atleast one
> 	regno in class is ok. Compute and use rclass size based on
> 	actually available regnos for mode in rclass.
>
> gcc/testsuite/ChangeLog:
>
> 2016-10-13  Senthil Kumar Selvaraj  <senthil_kumar.selvaraj@atmel.com>
> 	
> 	* gcc.target/avr/pr71627.c: New.
>
>
> Index: gcc/reload.c
> ===================================================================
> --- gcc/reload.c	(revision 240989)
> +++ gcc/reload.c	(working copy)
> @@ -711,31 +711,36 @@
>    enum reg_class best_class = NO_REGS;
>    unsigned int best_size = 0;
>    int cost;
> +  unsigned int computed_rclass_sizes[N_REG_CLASSES] = { 0 };

As far as I can tell you're only accessing this as 
computed_rclass_size[rclass], i.e. with the current class in the loop. 
So I don't think you need the array at all, just a computed_size 
variable in the loop?

> +      for (regno = 0; regno < FIRST_PSEUDO_REGISTER; regno++)
> +        {
> +          if (in_hard_reg_set_p (reg_class_contents[rclass], mode, regno))
> +            {
> +              atleast_one_regno_ok = 1;
> +              if (HARD_REGNO_MODE_OK (regno, mode))
> +                computed_rclass_sizes[rclass]++;
> +            }
> +        }

Don't you want to also ensure HARD_REGNO_MODE_OK before claiming that 
atleast_one_regno_ok? Maybe I'm forgetting the motivation but this seems 
odd. If so, the variable becomes unnecessary, just check the computed size.


Bernd
Senthil Kumar Selvaraj Oct. 18, 2016, 12:15 p.m. | #3
Bernd Schmidt writes:

> On 10/13/2016 08:57 AM, Senthil Kumar Selvaraj wrote:
>>
>> 2016-10-13  Senthil Kumar Selvaraj  <senthil_kumar.selvaraj@atmel.com>
>>
>> 	* reload.c (find_valid_class_1): Allow regclass if atleast one
>> 	regno in class is ok. Compute and use rclass size based on
>> 	actually available regnos for mode in rclass.
>>
>> gcc/testsuite/ChangeLog:
>>
>> 2016-10-13  Senthil Kumar Selvaraj  <senthil_kumar.selvaraj@atmel.com>
>> 	
>> 	* gcc.target/avr/pr71627.c: New.
>>
>>
>> Index: gcc/reload.c
>> ===================================================================
>> --- gcc/reload.c	(revision 240989)
>> +++ gcc/reload.c	(working copy)
>> @@ -711,31 +711,36 @@
>>    enum reg_class best_class = NO_REGS;
>>    unsigned int best_size = 0;
>>    int cost;
>> +  unsigned int computed_rclass_sizes[N_REG_CLASSES] = { 0 };
>
> As far as I can tell you're only accessing this as 
> computed_rclass_size[rclass], i.e. with the current class in the loop. 
> So I don't think you need the array at all, just a computed_size 
> variable in the loop?

Yes - I mechanically replaced the original array with the computed one.
A variable would suffice.
>
>> +      for (regno = 0; regno < FIRST_PSEUDO_REGISTER; regno++)
>> +        {
>> +          if (in_hard_reg_set_p (reg_class_contents[rclass], mode, regno))
>> +            {
>> +              atleast_one_regno_ok = 1;
>> +              if (HARD_REGNO_MODE_OK (regno, mode))
>> +                computed_rclass_sizes[rclass]++;
>> +            }
>> +        }
>
> Don't you want to also ensure HARD_REGNO_MODE_OK before claiming that 
> atleast_one_regno_ok? Maybe I'm forgetting the motivation but this seems 
> odd. If so, the variable becomes unnecessary, just check the computed size.

True again - the original intention was to prevent the best_xxx
variables from getting set if no regno was in_hard_reg_set. Now the
computed class size would be zero, so the variable is unnecessary.

Will do both the changes and re-run the reg tests. Ok for trunk if the
tests pass for x86_64-pc-linux and avr?

Regards
Senthil
Bernd Schmidt Oct. 18, 2016, 12:20 p.m. | #4
On 10/18/2016 02:15 PM, Senthil Kumar Selvaraj wrote:
> Will do both the changes and re-run the reg tests. Ok for trunk if the
> tests pass for x86_64-pc-linux and avr?
>
Probably but let's see the patch first.


Bernd

Patch

Index: gcc/reload.c
===================================================================
--- gcc/reload.c	(revision 240989)
+++ gcc/reload.c	(working copy)
@@ -711,31 +711,36 @@ 
   enum reg_class best_class = NO_REGS;
   unsigned int best_size = 0;
   int cost;
+  unsigned int computed_rclass_sizes[N_REG_CLASSES] = { 0 };
 
   for (rclass = 1; rclass < N_REG_CLASSES; rclass++)
     {
-      int bad = 0;
-      for (regno = 0; regno < FIRST_PSEUDO_REGISTER && !bad; regno++)
-	{
-	  if (in_hard_reg_set_p (reg_class_contents[rclass], mode, regno)
-	      && !HARD_REGNO_MODE_OK (regno, mode))
-	    bad = 1;
-	}
-      
-      if (bad)
-	continue;
+      int atleast_one_regno_ok = 0;
 
+      for (regno = 0; regno < FIRST_PSEUDO_REGISTER; regno++)
+        {
+          if (in_hard_reg_set_p (reg_class_contents[rclass], mode, regno))
+            {
+              atleast_one_regno_ok = 1;
+              if (HARD_REGNO_MODE_OK (regno, mode))
+                computed_rclass_sizes[rclass]++;
+            }
+        }
+
+      if (!atleast_one_regno_ok)
+        continue;
+
       cost = register_move_cost (outer, (enum reg_class) rclass, dest_class);
 
-      if ((reg_class_size[rclass] > best_size
-	   && (best_cost < 0 || best_cost >= cost))
-	  || best_cost > cost)
-	{
-	  best_class = (enum reg_class) rclass;
-	  best_size = reg_class_size[rclass];
-	  best_cost = register_move_cost (outer, (enum reg_class) rclass,
-					  dest_class);
-	}
+      if ((computed_rclass_sizes[rclass] > best_size
+	    && (best_cost < 0 || best_cost >= cost))
+	   || best_cost > cost)
+	 {
+	   best_class = (enum reg_class) rclass;
+	   best_size = computed_rclass_sizes[rclass];
+	   best_cost = register_move_cost (outer, (enum reg_class) rclass,
+					   dest_class);
+	 }
     }
 
   gcc_assert (best_size != 0);

Index: gcc/testsuite/gcc.target/avr/pr71627.c
===================================================================
--- gcc/testsuite/gcc.target/avr/pr71627.c      (nonexistent)
+++ gcc/testsuite/gcc.target/avr/pr71627.c      (working copy)
@@ -0,0 +1,14 @@ 
+/* { dg-do compile } */
+/* { dg-options "-O1" } */
+
+
+extern volatile __memx const long  a, b, c, d, e, f;
+extern volatile long result;
+
+extern void vfunc (const char*, ...);
+
+void foo (void)
+{
+       result = a + b + c + d + e + f;
+       vfunc ("text", a, b, c, d, e, f, result);
+}