Re: [Patch, reload, tentative, PR 71627] Tweak conditions in find_valid_class_1

[Bernd Schmidt]

On 10/21/2016 12:46 PM, Senthil Kumar Selvaraj wrote:

How does this look?


Looks good, thanks.


Bernd

Re: [Patch, reload, tentative, PR 71627] Tweak conditions in find_valid_class_1

[Senthil Kumar Selvaraj]

Bernd Schmidt writes:

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

How does this look?

Bootstrapped and reg tested x86_64-pc-linux on top of trunk@190252 with
the in_hard_reg_set_p patch backport - there were no failures. Also ran
regtests for avr on trunk, no failures there as well.

Ok to commit to trunk?

Regards
Senthil

gcc/ChangeLog:

2016-10-21  Senthil Kumar Selvaraj  

* 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-21  Senthil Kumar Selvaraj  

* gcc.target/avr/pr71627.c: New test.




diff --git gcc/reload.c gcc/reload.c
index 9a859e5..880099e 100644
--- gcc/reload.c
+++ gcc/reload.c
@@ -715,25 +715,23 @@ find_valid_class_1 (machine_mode outer ATTRIBUTE_UNUSED,
 
   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;
+  unsigned int computed_rclass_size = 0;
+
+  for (regno = 0; regno < FIRST_PSEUDO_REGISTER; regno++)
+{
+  if (in_hard_reg_set_p (reg_class_contents[rclass], mode, regno)
+  && (HARD_REGNO_MODE_OK (regno, mode)))
+computed_rclass_size++;
+}
 
   cost = register_move_cost (outer, (enum reg_class) rclass, dest_class);
 
-  if ((reg_class_size[rclass] > best_size
+  if ((computed_rclass_size > best_size
   && (best_cost < 0 || best_cost >= cost))
  || best_cost > cost)
{
  best_class = (enum reg_class) rclass;
- best_size = reg_class_size[rclass];
+ best_size = computed_rclass_size;
  best_cost = register_move_cost (outer, (enum reg_class) rclass,
  dest_class);
}
diff --git gcc/testsuite/gcc.target/avr/pr71627.c 
gcc/testsuite/gcc.target/avr/pr71627.c
new file mode 100644
index 000..eaef3d2
--- /dev/null
+++ gcc/testsuite/gcc.target/avr/pr71627.c
@@ -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);
+}

Re: [Patch, reload, tentative, PR 71627] Tweak conditions in find_valid_class_1

[Bernd Schmidt]

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

Re: [Patch, reload, tentative, PR 71627] Tweak conditions in find_valid_class_1

[Senthil Kumar Selvaraj]

Bernd Schmidt writes:

> On 10/13/2016 08:57 AM, Senthil Kumar Selvaraj wrote:
>>
>> 2016-10-13  Senthil Kumar Selvaraj  
>>
>>  * 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  
>>  
>>  * 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

Re: [Patch, reload, tentative, PR 71627] Tweak conditions in find_valid_class_1

[Bernd Schmidt]

On 10/13/2016 08:57 AM, Senthil Kumar Selvaraj wrote:


2016-10-13  Senthil Kumar Selvaraj  

* 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  

* 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

Re: [Patch, reload, tentative, PR 71627] Tweak conditions in find_valid_class_1

[Senthil Kumar Selvaraj]

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  
>
>   * 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  
>   
>   * 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);
> +}

Re: [Patch, reload, tentative, PR 71627] Tweak conditions in find_valid_class_1

[Senthil Kumar Selvaraj]

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  

* 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  

* 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);
+}

Re: [Patch, reload, tentative, PR 71627] Tweak conditions in find_valid_class_1

[Bernd Schmidt]

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.


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


Bernd

Re: [Patch, reload, tentative, PR 71627] Tweak conditions in find_valid_class_1

[Senthil Kumar Selvaraj]

Ping!

Regards
Senthil

Senthil Kumar Selvaraj writes:

> Hi,
>
>   The below patch fixes what I think are a couple of problems with
>   reload.c:find_valid_class_1.
>
>   First, even if no regno is in_hard_reg_set_p, it goes ahead and
>   considers rclass as valid. bad is set only if a regno is in the reg
>   class *and* HARD_REGNO_MODE_OK is false - if both are false, bad isn't
>   set and the reload gets a wrong rclass. If that happens to be the best
>   one, this eventually leads to find_reg running out of registers to
>   spill, because the chosen rclass won't have enough regs.
>
>   Second, it expects every regno in rclass to be valid for mode i.e., if
>   any regno fails HARD_REGNO_MODE_OK, it rejects the rclass. The
>   comments in the original commit for find_valid_class_1 say atleast one
>   regno is ok. This was updated to say "class which contains only
>   registers" when in_hard_reg_set_p was introduced in place of just
>   TEST_HARD_REG_BIT.
>
>   Is it meant to really test all regs? For the avr target, all regs are
>   8 bits wide, and HARD_REGNO_MODE_OK returns false for odd regnos if
>   mode != QImode. With the current code, it ignores a bunch of otherwise
>   perfectly legal reg classes.
>
>   To fix the first problem, the patch adds regno_in_rclass_mode to track
>   whether there's atleast one regno in hard_reg_set_p. To fix the second
>   problem, the patch sets bad initially, and resets it if
>   HARD_REGNO_MODE_OK succeeded for a regno, thus breaking the loop.
>
>   Of course, if both my points above are valid, we can do away with
>   regno_in_rclass_mode, just bad would do.
>
>   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.
>
> Regards
> Senthil
>
>
> Index: reload.c
> ===
> --- reload.c  (revision 240185)
> +++ reload.c  (working copy)
> @@ -714,17 +714,22 @@
>  
>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 bad = 1;
> +  int regno_in_rclass_mode = 0;
>  
> +  for (regno = 0; regno < FIRST_PSEUDO_REGISTER && bad; regno++)
> +{
> +  if (in_hard_reg_set_p (reg_class_contents[rclass], mode, regno))
> +{
> +  regno_in_rclass_mode = 1;
> +  if (HARD_REGNO_MODE_OK (regno, mode))
> +bad = 0;
> +}
> +}
> +
> +  if (bad || !regno_in_rclass_mode)
> +continue;
> +
>cost = register_move_cost (outer, (enum reg_class) rclass, dest_class);
>  
>if ((reg_class_size[rclass] > best_size

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

[Senthil Kumar Selvaraj]

Hi,

  The below patch fixes what I think are a couple of problems with
  reload.c:find_valid_class_1.

  First, even if no regno is in_hard_reg_set_p, it goes ahead and
  considers rclass as valid. bad is set only if a regno is in the reg
  class *and* HARD_REGNO_MODE_OK is false - if both are false, bad isn't
  set and the reload gets a wrong rclass. If that happens to be the best
  one, this eventually leads to find_reg running out of registers to
  spill, because the chosen rclass won't have enough regs.

  Second, it expects every regno in rclass to be valid for mode i.e., if
  any regno fails HARD_REGNO_MODE_OK, it rejects the rclass. The
  comments in the original commit for find_valid_class_1 say atleast one
  regno is ok. This was updated to say "class which contains only
  registers" when in_hard_reg_set_p was introduced in place of just
  TEST_HARD_REG_BIT.

  Is it meant to really test all regs? For the avr target, all regs are
  8 bits wide, and HARD_REGNO_MODE_OK returns false for odd regnos if
  mode != QImode. With the current code, it ignores a bunch of otherwise
  perfectly legal reg classes.

  To fix the first problem, the patch adds regno_in_rclass_mode to track
  whether there's atleast one regno in hard_reg_set_p. To fix the second
  problem, the patch sets bad initially, and resets it if
  HARD_REGNO_MODE_OK succeeded for a regno, thus breaking the loop.

  Of course, if both my points above are valid, we can do away with
  regno_in_rclass_mode, just bad would do.

  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.

Regards
Senthil


Index: reload.c
===
--- reload.c(revision 240185)
+++ reload.c(working copy)
@@ -714,17 +714,22 @@
 
   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 bad = 1;
+  int regno_in_rclass_mode = 0;
 
+  for (regno = 0; regno < FIRST_PSEUDO_REGISTER && bad; regno++)
+{
+  if (in_hard_reg_set_p (reg_class_contents[rclass], mode, regno))
+{
+  regno_in_rclass_mode = 1;
+  if (HARD_REGNO_MODE_OK (regno, mode))
+bad = 0;
+}
+}
+
+  if (bad || !regno_in_rclass_mode)
+continue;
+
   cost = register_move_cost (outer, (enum reg_class) rclass, dest_class);
 
   if ((reg_class_size[rclass] > best_size