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

Submitted by Senthil Kumar Selvaraj on Sept. 16, 2016, 7:02 p.m.

Details

Message ID 87lgyris6r.fsf@atmel.com
State New
Headers show

Commit Message

Senthil Kumar Selvaraj Sept. 16, 2016, 7:02 p.m.
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

Comments

Senthil Kumar Selvaraj Sept. 26, 2016, 12:31 p.m.
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
Bernd Schmidt Sept. 26, 2016, 3:45 p.m.
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

Patch hide | download patch | download mbox

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