Patchwork patch to fix PR21617

login
register
mail settings
Submitter Vladimir Makarov
Date Dec. 12, 2011, 9:04 p.m.
Message ID <4EE66C62.3060705@redhat.com>
Download mbox | patch
Permalink /patch/130846/
State New
Headers show

Comments

Vladimir Makarov - Dec. 12, 2011, 9:04 p.m.
The following patch solves PR 21617 which is described on 
http://gcc.gnu.org/bugzilla/show_bug.cgi?id=21617.

Just adding number of necessary hard registers solves the problem but 
creates 2% SPEC2000 perlbmk degradation on x86.  Fortunately, removing 
allocno class comparison removes the degradation.   The allocno class 
comparison was originally added for debugging purposes to put coloring 
of allocnos of the same class in one place.  It was ok when we had cover 
classes which did not intersect.  Now allocno classes intersect and this 
comparison should be not used (at least as highest priority heuristic).

Beside solving the problem, the patch improves a bit SPEC2000 
performance and code size on x86.

The patch was successfully bootstrapped on x86/x86-64.  I expect it 
should be pretty safe for other targets because it changes heuristics only.

Committed as rev. 182263.


2011-12-12  Vladimir Makarov <vmakarov@redhat.com>

         PR rtl-optimization/21617
         * ira-color.c (bucket_allocno_compare_func): Don't compare
         allocno classes.  Compare number of hard registers needed.
Ilya Enkovich - Dec. 22, 2011, 11:19 a.m.
2011/12/13 Vladimir Makarov <vmakarov@redhat.com>:
> The following patch solves PR 21617 which is described on
> http://gcc.gnu.org/bugzilla/show_bug.cgi?id=21617.
>
> Just adding number of necessary hard registers solves the problem but
> creates 2% SPEC2000 perlbmk degradation on x86.  Fortunately, removing
> allocno class comparison removes the degradation.   The allocno class
> comparison was originally added for debugging purposes to put coloring of
> allocnos of the same class in one place.  It was ok when we had cover
> classes which did not intersect.  Now allocno classes intersect and this
> comparison should be not used (at least as highest priority heuristic).
>
> Beside solving the problem, the patch improves a bit SPEC2000 performance
> and code size on x86.
>
> The patch was successfully bootstrapped on x86/x86-64.  I expect it should
> be pretty safe for other targets because it changes heuristics only.
>
> Committed as rev. 182263.
>
>
> 2011-12-12  Vladimir Makarov <vmakarov@redhat.com>
>
>        PR rtl-optimization/21617
>        * ira-color.c (bucket_allocno_compare_func): Don't compare
>        allocno classes.  Compare number of hard registers needed.
>

Hello, Vladimir,

I think there may be a typo in this patch. I saw few degradations in
EEMBC2.0 benchmarks caused by this patch. Following fix removes these
degradations:

-  if ((diff = (ira_reg_class_max_nregs[cl1][ALLOCNO_MODE (a1)]
-              - ira_reg_class_max_nregs[cl2][ALLOCNO_MODE (a2)])) != 0)
+  if ((diff = (ira_reg_class_max_nregs[cl2][ALLOCNO_MODE (a2)]
+              - ira_reg_class_max_nregs[cl1][ALLOCNO_MODE (a1)])) != 0)

I suspect typo because previous calculation of 'diff' value used f(a2)
- f(a1) formula and now it is f(a1) - f(a2). Could you please look at
it?

Thanks,
Ilya
Vladimir Makarov - Dec. 22, 2011, 4:15 p.m.
On 12/22/2011 06:19 AM, Ilya Enkovich wrote:
> 2011/12/13 Vladimir Makarov<vmakarov@redhat.com>:
>> The following patch solves PR 21617 which is described on
>> http://gcc.gnu.org/bugzilla/show_bug.cgi?id=21617.
>>
>> Just adding number of necessary hard registers solves the problem but
>> creates 2% SPEC2000 perlbmk degradation on x86.  Fortunately, removing
>> allocno class comparison removes the degradation.   The allocno class
>> comparison was originally added for debugging purposes to put coloring of
>> allocnos of the same class in one place.  It was ok when we had cover
>> classes which did not intersect.  Now allocno classes intersect and this
>> comparison should be not used (at least as highest priority heuristic).
>>
>> Beside solving the problem, the patch improves a bit SPEC2000 performance
>> and code size on x86.
>>
>> The patch was successfully bootstrapped on x86/x86-64.  I expect it should
>> be pretty safe for other targets because it changes heuristics only.
>>
>> Committed as rev. 182263.
>>
>>
>> 2011-12-12  Vladimir Makarov<vmakarov@redhat.com>
>>
>>         PR rtl-optimization/21617
>>         * ira-color.c (bucket_allocno_compare_func): Don't compare
>>         allocno classes.  Compare number of hard registers needed.
>>
> Hello, Vladimir,
>
> I think there may be a typo in this patch. I saw few degradations in
> EEMBC2.0 benchmarks caused by this patch. Following fix removes these
> degradations:
>
> -  if ((diff = (ira_reg_class_max_nregs[cl1][ALLOCNO_MODE (a1)]
> -              - ira_reg_class_max_nregs[cl2][ALLOCNO_MODE (a2)])) != 0)
> +  if ((diff = (ira_reg_class_max_nregs[cl2][ALLOCNO_MODE (a2)]
> +              - ira_reg_class_max_nregs[cl1][ALLOCNO_MODE (a1)])) != 0)
>
> I suspect typo because previous calculation of 'diff' value used f(a2)
> - f(a1) formula and now it is f(a1) - f(a2). Could you please look at
> it?
>
I don't think it was a typo.  The patch puts multi-registers pseudos at 
the end of the coloring bucket, so they will push to the coloring stacke 
last and popped from the stack first and get better chance to be 
assigned to hard registers because smaller chance of small holes 
creation of free hard registers.

Generally speaking, RA is all about heuristics and it is always possible 
to find a test when one heuristic will work better than another one and 
vise versa.  So for me, there is a little sense to work on such PRs of 
course unless it results in a discovery of better heuristics which 
improves overall score on a credible test set like SPECCPU, for example.

I work on these PRs mostly to help next gcc release to come out.

Returning to your complaint, could you give me info how the overall 
score of EEMBC changed after committing the patch, not just saying that 
there are few degradations.

Patch

Index: ira-color.c
===================================================================
--- ira-color.c	(revision 182262)
+++ ira-color.c	(working copy)
@@ -1797,8 +1797,14 @@  bucket_allocno_compare_func (const void 
   ira_allocno_t a1 = *(const ira_allocno_t *) v1p;
   ira_allocno_t a2 = *(const ira_allocno_t *) v2p;
   int diff, a1_freq, a2_freq, a1_num, a2_num;
+  int cl1 = ALLOCNO_CLASS (a1), cl2 = ALLOCNO_CLASS (a2);
 
-  if ((diff = (int) ALLOCNO_CLASS (a2) - ALLOCNO_CLASS (a1)) != 0)
+  /* Push pseudos requiring less hard registers first.  It means that
+     we will assign pseudos requiring more hard registers first
+     avoiding creation small holes in free hard register file into
+     which the pseudos requiring more hard registers can not fit.  */
+  if ((diff = (ira_reg_class_max_nregs[cl1][ALLOCNO_MODE (a1)]
+	       - ira_reg_class_max_nregs[cl2][ALLOCNO_MODE (a2)])) != 0)
     return diff;
   a1_freq = ALLOCNO_FREQ (a1);
   a2_freq = ALLOCNO_FREQ (a2);