Re: FW: patch to fix PR21617

Re: FW: patch to fix PR21617

[Igor Zamyatin]

Ilya is on vacation so I'll make the answer.

Overall score became worse on 0.3%.

> -----Original Message-----
> From: gcc-patches-owner@gcc.gnu.org [mailto:gcc-patches-owner@gcc.gnu.org] On Behalf Of Vladimir Makarov
> Sent: Thursday, December 22, 2011 8:15 PM
> To: Ilya Enkovich
> Cc: gcc-patches
> Subject: Re: patch to fix PR21617
>
> 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.

Re: FW: patch to fix PR21617

[Vladimir Makarov]

On 12/29/2011 06:41 AM, Igor Zamyatin wrote:
> Ilya is on vacation so I'll make the answer.
>
> Overall score became worse on 0.3%.
>
Ok, thanks.  It is in the range of measure error for some processors.  
But Intel processors range is pretty small.

Did you use Atom for measuring?

I'll try to find another solution for the problem.  But I need a help 
for benchmarking patches as I have no access to EEMBC.

Re: FW: patch to fix PR21617

[Vladimir Makarov]

On 01/18/2012 02:30 PM, Zamyatin, Igor wrote:
> Yes, we use Atom for EEMBC measurements.
>
> We'll be glad to help you with your findings.
>
>
Thanks.

Unfortunately I tried several alternative patches but I did not find a 
better solution (it is mostly code size degradation on CoreI7).  Now I 
am even thinking that the best action would have been ignoring the 
original PR.

Re: FW: patch to fix PR21617

[Vladimir Makarov]

[-- Attachment #1: Type: text/plain, Size: 626 bytes --]

On 01/19/2012 03:52 PM, Vladimir Makarov wrote:
> On 01/18/2012 02:30 PM, Zamyatin, Igor wrote:
>> Yes, we use Atom for EEMBC measurements.
>>
>> We'll be glad to help you with your findings.
>>
>>
> Thanks.
>
> Unfortunately I tried several alternative patches but I did not find a 
> better solution (it is mostly code size degradation on CoreI7).  Now I 
> am even thinking that the best action would have been ignoring the 
> original PR.
>
Could you try the attached patch.  It might work.

I've tried several approach to prevent small hole creation in 
ira-color.c::assign_hard_reg but it does not work well.

Thanks.



[-- Attachment #2: pr21617-2.patch --]
[-- Type: text/x-patch, Size: 1434 bytes --]

Index: ira-color.c
===================================================================
--- ira-color.c	(revision 183312)
+++ ira-color.c	(working copy)
@@ -1798,16 +1798,16 @@ bucket_allocno_compare_func (const void 
   int diff, a1_freq, a2_freq, a1_num, a2_num;
   int cl1 = ALLOCNO_CLASS (a1), cl2 = ALLOCNO_CLASS (a2);
 
-  /* 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);
-  if ((diff = a1_freq - a2_freq) != 0)
+  /* Prefer to 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 = (a1_freq * ira_reg_class_max_nregs[cl1][ALLOCNO_MODE (a1)]
+	       - a2_freq * ira_reg_class_max_nregs[cl2][ALLOCNO_MODE (a2)]))
+      != 0)
     return diff;
   a1_num = ALLOCNO_COLOR_DATA (a1)->available_regs_num;
   a2_num = ALLOCNO_COLOR_DATA (a2)->available_regs_num;

Re: FW: patch to fix PR21617

[Igor Zamyatin]

Unfortunately patch doesn't help neither for separate EEMBC_2_0 tests
nor for the whole benchmark.

Do you want me to do some debugging here?

On Fri, Jan 20, 2012 at 10:13 PM, Vladimir Makarov <vmakarov@redhat.com> wrote:
> On 01/19/2012 03:52 PM, Vladimir Makarov wrote:
>>
>> On 01/18/2012 02:30 PM, Zamyatin, Igor wrote:
>>>
>>> Yes, we use Atom for EEMBC measurements.
>>>
>>> We'll be glad to help you with your findings.
>>>
>>>
>> Thanks.
>>
>> Unfortunately I tried several alternative patches but I did not find a
>> better solution (it is mostly code size degradation on CoreI7).  Now I am
>> even thinking that the best action would have been ignoring the original PR.
>>
> Could you try the attached patch.  It might work.
>
> I've tried several approach to prevent small hole creation in
> ira-color.c::assign_hard_reg but it does not work well.
>
> Thanks.
>
>

Re: FW: patch to fix PR21617

[Vladimir Makarov]

On 01/23/2012 06:32 AM, Igor Zamyatin wrote:
> Unfortunately patch doesn't help neither for separate EEMBC_2_0 tests
> nor for the whole benchmark.
>
> Do you want me to do some debugging here?
>
For now I am out of ideas how to fix the PR in alternative way without 
some performance degradation on SPEC or EEMBC.

Any reduced test case from EEMBC where I can see performance degradation 
might help me too.