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

[Senthil Kumar Selvaraj <senthil_kumar dot selvaraj at atmel dot com>]

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