* Re: FW: patch to fix PR21617
[not found] <0EFAB2BDD0F67E4FB6CCC8B9F87D75690EC81F@IRSMSX101.ger.corp.intel.com>
@ 2011-12-29 11:59 ` Igor Zamyatin
2012-01-10 17:44 ` Vladimir Makarov
0 siblings, 1 reply; 6+ messages in thread
From: Igor Zamyatin @ 2011-12-29 11:59 UTC (permalink / raw)
To: vmakarov; +Cc: enkovich.gnu, izamyatin, igor.zamyatin, gcc-patches
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.
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: FW: patch to fix PR21617
2011-12-29 11:59 ` FW: patch to fix PR21617 Igor Zamyatin
@ 2012-01-10 17:44 ` Vladimir Makarov
[not found] ` <0EFAB2BDD0F67E4FB6CCC8B9F87D756910D3FD@IRSMSX101.ger.corp.intel.com>
0 siblings, 1 reply; 6+ messages in thread
From: Vladimir Makarov @ 2012-01-10 17:44 UTC (permalink / raw)
To: Igor Zamyatin; +Cc: enkovich.gnu, igor.zamyatin, gcc-patches
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.
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: FW: patch to fix PR21617
[not found] ` <0EFAB2BDD0F67E4FB6CCC8B9F87D756910D3FD@IRSMSX101.ger.corp.intel.com>
@ 2012-01-19 20:53 ` Vladimir Makarov
2012-01-20 18:15 ` Vladimir Makarov
0 siblings, 1 reply; 6+ messages in thread
From: Vladimir Makarov @ 2012-01-19 20:53 UTC (permalink / raw)
To: Zamyatin, Igor; +Cc: Igor Zamyatin, enkovich.gnu, gcc-patches
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.
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: FW: patch to fix PR21617
2012-01-19 20:53 ` Vladimir Makarov
@ 2012-01-20 18:15 ` Vladimir Makarov
2012-01-23 11:32 ` Igor Zamyatin
0 siblings, 1 reply; 6+ messages in thread
From: Vladimir Makarov @ 2012-01-20 18:15 UTC (permalink / raw)
To: Zamyatin, Igor; +Cc: Igor Zamyatin, enkovich.gnu, gcc-patches
[-- 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;
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: FW: patch to fix PR21617
2012-01-20 18:15 ` Vladimir Makarov
@ 2012-01-23 11:32 ` Igor Zamyatin
2012-01-25 16:45 ` Vladimir Makarov
0 siblings, 1 reply; 6+ messages in thread
From: Igor Zamyatin @ 2012-01-23 11:32 UTC (permalink / raw)
To: Vladimir Makarov; +Cc: Zamyatin, Igor, enkovich.gnu, gcc-patches
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.
>
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: FW: patch to fix PR21617
2012-01-23 11:32 ` Igor Zamyatin
@ 2012-01-25 16:45 ` Vladimir Makarov
0 siblings, 0 replies; 6+ messages in thread
From: Vladimir Makarov @ 2012-01-25 16:45 UTC (permalink / raw)
To: Igor Zamyatin; +Cc: Zamyatin, Igor, enkovich.gnu, gcc-patches
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.
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2012-01-25 16:45 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
[not found] <0EFAB2BDD0F67E4FB6CCC8B9F87D75690EC81F@IRSMSX101.ger.corp.intel.com>
2011-12-29 11:59 ` FW: patch to fix PR21617 Igor Zamyatin
2012-01-10 17:44 ` Vladimir Makarov
[not found] ` <0EFAB2BDD0F67E4FB6CCC8B9F87D756910D3FD@IRSMSX101.ger.corp.intel.com>
2012-01-19 20:53 ` Vladimir Makarov
2012-01-20 18:15 ` Vladimir Makarov
2012-01-23 11:32 ` Igor Zamyatin
2012-01-25 16:45 ` Vladimir Makarov
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).