Patchwork [IRA] Fix PR rtl-optimization/45912

login
register
mail settings
Submitter Eric Botcazou
Date Oct. 12, 2010, 10:03 p.m.
Message ID <201010130003.42922.ebotcazou@adacore.com>
Download mbox | patch
Permalink /patch/67628/
State New
Headers show

Comments

Eric Botcazou - Oct. 12, 2010, 10:03 p.m.
Hi,

the newly added code in ira_tune_allocno_costs_and_cover_classes dealing with 
unaligned registers:

      /* Some targets allow pseudos to be allocated to unaligned
         sequences of hard registers.  However, selecting an unaligned
         sequence can unnecessarily restrict later allocations.  So
         increase the cost of unaligned hard regs to encourage the use
         of aligned hard regs.  */

makes an implicit assumption about the mapping of hard regs to indices within 
classes:

	    for (j = n - 1; j >= 0; j--)
	      {
		if (j % nregs != 0)
		  {
		    regno = ira_non_ordered_class_hard_regs[cover_class][j];

The unalignment test is applied to the indices instead of the regnos so it 
implicitly assumes that the mapping preserves the alignment.  This is wrong 
on the SPARC for GENERAL_REGS as the first register %g0 is hardwired to 0 so 
the register with index 0 is %g1.  This leads to quite poor allocation for 
32-bit code manipulating double words, for example the compiler itself.

Bootstrapped/regtested on SPARC/Solaris 8, OK for mainline?


2010-10-12  Eric Botcazou  <ebotcazou@adacore.com>

	PR rtl-optimization/45912
	* ira-costs.c (ira_tune_allocno_costs_and_cover_classes): Test the
	regno of registers instead of their index to compute the alignment.



2010-10-12  Eric Botcazou  <ebotcazou@adacore.com>

	* gcc.c-torture/execute/20101012-1.c: New test.
Vladimir Makarov - Oct. 13, 2010, 4:22 p.m.
On 10/12/2010 06:03 PM, Eric Botcazou wrote:
> Hi,
>
> the newly added code in ira_tune_allocno_costs_and_cover_classes dealing with
> unaligned registers:
>
>        /* Some targets allow pseudos to be allocated to unaligned
>           sequences of hard registers.  However, selecting an unaligned
>           sequence can unnecessarily restrict later allocations.  So
>           increase the cost of unaligned hard regs to encourage the use
>           of aligned hard regs.  */
>
> makes an implicit assumption about the mapping of hard regs to indices within
> classes:
>
> 	    for (j = n - 1; j>= 0; j--)
> 	      {
> 		if (j % nregs != 0)
> 		  {
> 		    regno = ira_non_ordered_class_hard_regs[cover_class][j];
>
> The unalignment test is applied to the indices instead of the regnos so it
> implicitly assumes that the mapping preserves the alignment.  This is wrong
> on the SPARC for GENERAL_REGS as the first register %g0 is hardwired to 0 so
> the register with index 0 is %g1.  This leads to quite poor allocation for
> 32-bit code manipulating double words, for example the compiler itself.
>
> Bootstrapped/regtested on SPARC/Solaris 8, OK for mainline?
>
>
Yes.  Thanks for working on the PR, Eric.
> 2010-10-12  Eric Botcazou<ebotcazou@adacore.com>
>
> 	PR rtl-optimization/45912
> 	* ira-costs.c (ira_tune_allocno_costs_and_cover_classes): Test the
> 	regno of registers instead of their index to compute the alignment.
>
>
>
> 2010-10-12  Eric Botcazou<ebotcazou@adacore.com>
>
> 	* gcc.c-torture/execute/20101012-1.c: New test.
>
>
Eric Botcazou - Oct. 13, 2010, 5:39 p.m.
> > Bootstrapped/regtested on SPARC/Solaris 8, OK for mainline?
>
> Yes.  Thanks for working on the PR, Eric.

Thanks.  I was wondering if ira_non_ordered_class_hard_regs isn't superfluous 
now that we iterate over the whole class.  Can't we just do:

 	    for (j = n - 1; j >= 0; j--)
 	      {
		regno = ira_class_hard_regs[cover_class][j];
		if ((regno % nregs) != 0)
 		   reg_costs[j] += ALLOCNO_FREQ (a);
	      }

and get rid of ira_non_ordered_class_hard_regs entirely?  FWIW here are their 
contents for GENERAL_REGS on the SPARC:

(gdb) p default_target_ira->x_ira_class_hard_regs[GENERAL_REGS]
$5 = {1, 2, 3, 4, 29, 28, 27, 26, 25, 24, 15, 13, 12, 11, 10, 9, 8, 16, 17,
  18, 19, 20, 21, 22, 23, 0 <repeats 77 times>}

(gdb) p 
default_target_ira_int->x_ira_non_ordered_class_hard_regs[GENERAL_REGS]
$6 = {1, 2, 3, 4, 8, 9, 10, 11, 12, 13, 15, 16, 17, 18, 19, 20, 21, 22, 23,
  24, 25, 26, 27, 28, 29, 0 <repeats 77 times>}
Vladimir Makarov - Oct. 13, 2010, 8:48 p.m.
On 10/13/2010 01:39 PM, Eric Botcazou wrote:
>>> Bootstrapped/regtested on SPARC/Solaris 8, OK for mainline?
>> Yes.  Thanks for working on the PR, Eric.
> Thanks.  I was wondering if ira_non_ordered_class_hard_regs isn't superfluous
> now that we iterate over the whole class.  Can't we just do:
>
>   	    for (j = n - 1; j>= 0; j--)
>   	      {
> 		regno = ira_class_hard_regs[cover_class][j];
> 		if ((regno % nregs) != 0)
>   		   reg_costs[j] += ALLOCNO_FREQ (a);
> 	      }
>
Yes, we could remove it.  But I think it is better to stay.  Coming back 
to your previous patch, the problem occurs because 
ira_non_ordered_class_hard_regs contains only allocatable hard regs.  If 
it contains all hard regs, there were no such PR.  Although such change 
would need other changes (like supporting some consistency with 
ira_class_hard_reg_index and may be others).

I still believe that alignment should be taken not from regno but by 
order number of the regno in its cover class because sometimes hard 
registers are enumerated as follows

one class hard registers, some virtual hard regs, another class hard regs

and for the second class hard regs, your approach will not work.

So I'd like to consider your previous patch as a temporary solution 
(although some temporary solutions stay forever :).  I think that 
solution I wrote here is better but not ideal too (for example in a 
situation when a cover class is union of some other classes).  I'll put 
this work on my TODO list but honestly don't know when I start working 
on this.

So far your patch solves the problem and should stay in the trunk.

Thanks.
> and get rid of ira_non_ordered_class_hard_regs entirely?  FWIW here are their
> contents for GENERAL_REGS on the SPARC:
>
> (gdb) p default_target_ira->x_ira_class_hard_regs[GENERAL_REGS]
> $5 = {1, 2, 3, 4, 29, 28, 27, 26, 25, 24, 15, 13, 12, 11, 10, 9, 8, 16, 17,
>    18, 19, 20, 21, 22, 23, 0<repeats 77 times>}
>
> (gdb) p
> default_target_ira_int->x_ira_non_ordered_class_hard_regs[GENERAL_REGS]
> $6 = {1, 2, 3, 4, 8, 9, 10, 11, 12, 13, 15, 16, 17, 18, 19, 20, 21, 22, 23,
>    24, 25, 26, 27, 28, 29, 0<repeats 77 times>}
>
Eric Botcazou - Oct. 13, 2010, 9:38 p.m.
> So I'd like to consider your previous patch as a temporary solution
> (although some temporary solutions stay forever :).  I think that
> solution I wrote here is better but not ideal too (for example in a
> situation when a cover class is union of some other classes).  I'll put
> this work on my TODO list but honestly don't know when I start working
> on this.

OK, thanks for the explanation.

Btw, there is a strange loop in setup_class_hard_regs:

      for (i = 0; i < FIRST_PSEUDO_REGISTER; i++)
	{
	  ira_non_ordered_class_hard_regs[cl][0] = -1;
	  ira_class_hard_reg_index[cl][0] = -1;
	}
Vladimir Makarov - Oct. 14, 2010, 3:11 a.m.
On 10/13/2010 05:38 PM, Eric Botcazou wrote:
>> So I'd like to consider your previous patch as a temporary solution
>> (although some temporary solutions stay forever :).  I think that
>> solution I wrote here is better but not ideal too (for example in a
>> situation when a cover class is union of some other classes).  I'll put
>> this work on my TODO list but honestly don't know when I start working
>> on this.
> OK, thanks for the explanation.
>
> Btw, there is a strange loop in setup_class_hard_regs:
>
>        for (i = 0; i<  FIRST_PSEUDO_REGISTER; i++)
> 	{
> 	  ira_non_ordered_class_hard_regs[cl][0] = -1;
> 	  ira_class_hard_reg_index[cl][0] = -1;
> 	}
>
Obviously, it is a typo.  I've just fixed it on the trunk.

Thanks for pointing it out.

Patch

Index: ira-costs.c
===================================================================
--- ira-costs.c	(revision 165234)
+++ ira-costs.c	(working copy)
@@ -1787,15 +1787,14 @@  ira_tune_allocno_costs_and_cover_classes
       if (min_cost != INT_MAX)
 	ALLOCNO_COVER_CLASS_COST (a) = min_cost;
 
-      /* Some targets allow pseudos to be allocated to unaligned
-         sequences of hard registers.  However, selecting an unaligned
-         sequence can unnecessarily restrict later allocations.  So
-         increase the cost of unaligned hard regs to encourage the use
-         of aligned hard regs.  */
+      /* Some targets allow pseudos to be allocated to unaligned sequences
+	 of hard registers.  However, selecting an unaligned sequence can
+	 unnecessarily restrict later allocations.  So increase the cost of
+	 unaligned hard regs to encourage the use of aligned hard regs.  */
       {
-	int nregs, index;
+	const int nregs = ira_reg_class_nregs[cover_class][ALLOCNO_MODE (a)];
 
-	if ((nregs = ira_reg_class_nregs[cover_class][ALLOCNO_MODE (a)]) > 1)
+	if (nregs > 1)
 	  {
 	    ira_allocate_and_set_costs
 	      (&ALLOCNO_HARD_REG_COSTS (a), cover_class,
@@ -1803,10 +1802,10 @@  ira_tune_allocno_costs_and_cover_classes
 	    reg_costs = ALLOCNO_HARD_REG_COSTS (a);
 	    for (j = n - 1; j >= 0; j--)
 	      {
-		if (j % nregs != 0)
+		regno = ira_non_ordered_class_hard_regs[cover_class][j];
+		if ((regno % nregs) != 0)
 		  {
-		    regno = ira_non_ordered_class_hard_regs[cover_class][j];
-		    index = ira_class_hard_reg_index[cover_class][regno];
+		    int index = ira_class_hard_reg_index[cover_class][regno];
 		    ira_assert (index != -1);
 		    reg_costs[index] += ALLOCNO_FREQ (a);
 		  }