Comments

On Fri, Jul 30, 2010 at 5:03 PM, Bernd Schmidt <bernds@codesourcery.com> wrote:
> On 07/31/2010 01:07 AM, Martin Thuresson wrote:>> This patch updates the handling of temporary registers in postreload.c>> to avoid very long build times in certain files.>>>> By creating new registers instead of updating one existing one using>> SET_REGNO it avoids going through the scan df structure.>>>> Im new to gcc RTL and appreciate any feedback.>>>> In the (inspired from real code) attached source, the time>> for "reload CSE regs" went down from 20 usr seconds to less than 1.>> I have trouble reproducing the slowness. Which target are you using?
It seems that I did not include the larger testcase I had. I attached
it and these are the build times I measured:
Target: x86_64-unknown-linux-gnu
Options: -g -O2 -ftime-report -c
Before:
reload CSE regs : 13.65 (81%) usr 0.01 (25%) sys 13.73
(81%) wall 2812 kB ( 4%) ggc
After:
reload CSE regs : 0.16 ( 6%) usr 0.01 (14%) sys 0.17 ( 6%)
wall 2812 kB ( 4%) ggc
>> It's a bit unfortunate, and probably avoidable, to create additional> garbage RTL. Maybe what's really needed is> df_set_flags (DF_DEFER_INSN_RESCAN);> at the top of postreload? Can you try that? Or, even simpler, avoid> the SET_REGNO macro, and check all other occurrences of it. There seem> to be a few, e.g. in caller-save.c or ira.c, which also probably> shouldn't invoke df.
I updated the patch by introducing SET_REGNO_RAW and updated the
postreload.c, caller-save.c and ira.c.
I saw no new test failures.
Thanks,
Martin

On 08/04/2010 08:39 PM, Martin Thuresson wrote:
> I updated the patch by introducing SET_REGNO_RAW and updated the> postreload.c, caller-save.c and ira.c.> > I saw no new test failures.
Thanks for the patch!
Some general rules for patch submission: state exactly on which target
you bootstrapped and regression tested. You'll need a ChangeLog entry
which should be contained as plain text before the patch itself; see the
existing ChangeLog for examples, and
http://www.gnu.org/prep/standards/standards.html
for exactly how to write them.
Do you have commit access? I'm assuming Google has some kind of blanket
assignment in place.
Bernd

On 08/04/10 12:39, Martin Thuresson wrote:
> On Fri, Jul 30, 2010 at 5:03 PM, Bernd Schmidt<bernds@codesourcery.com> wrote:>> On 07/31/2010 01:07 AM, Martin Thuresson wrote:>>> This patch updates the handling of temporary registers in postreload.c>>> to avoid very long build times in certain files.>>>>>> By creating new registers instead of updating one existing one using>>> SET_REGNO it avoids going through the scan df structure.>>>>>> Im new to gcc RTL and appreciate any feedback.>>>>>> In the (inspired from real code) attached source, the time>>> for "reload CSE regs" went down from 20 usr seconds to less than 1.>> I have trouble reproducing the slowness. Which target are you using?> It seems that I did not include the larger testcase I had. I attached> it and these are the build times I measured:>> Target: x86_64-unknown-linux-gnu> Options: -g -O2 -ftime-report -c>> Before:> reload CSE regs : 13.65 (81%) usr 0.01 (25%) sys 13.73> (81%) wall 2812 kB ( 4%) ggc> After:> reload CSE regs : 0.16 ( 6%) usr 0.01 (14%) sys 0.17 ( 6%)> wall 2812 kB ( 4%) ggc>>>> It's a bit unfortunate, and probably avoidable, to create additional>> garbage RTL. Maybe what's really needed is>> df_set_flags (DF_DEFER_INSN_RESCAN);>> at the top of postreload? Can you try that? Or, even simpler, avoid>> the SET_REGNO macro, and check all other occurrences of it. There seem>> to be a few, e.g. in caller-save.c or ira.c, which also probably>> shouldn't invoke df.> I updated the patch by introducing SET_REGNO_RAW and updated the> postreload.c, caller-save.c and ira.c.>> I saw no new test failures.
I was going to look at your patch, but you only included the
SET_REGNO_RAW changes. It's customary to repost the entire patch after
you update it to address issues from reviewers.
jeff

On Thu, Aug 5, 2010 at 2:14 PM, Jeff Law <law@redhat.com> wrote:
> On 08/04/10 12:39, Martin Thuresson wrote:>>>> On Fri, Jul 30, 2010 at 5:03 PM, Bernd Schmidt<bernds@codesourcery.com>>> wrote:>>>>>> On 07/31/2010 01:07 AM, Martin Thuresson wrote:>>>>>>>> This patch updates the handling of temporary registers in postreload.c>>>> to avoid very long build times in certain files.>>>>>>>> By creating new registers instead of updating one existing one using>>>> SET_REGNO it avoids going through the scan df structure.>>>>>>>> Im new to gcc RTL and appreciate any feedback.>>>>>>>> In the (inspired from real code) attached source, the time>>>> for "reload CSE regs" went down from 20 usr seconds to less than 1.>>>>>> I have trouble reproducing the slowness. Which target are you using?>>>> It seems that I did not include the larger testcase I had. I attached>> it and these are the build times I measured:>>>> Target: x86_64-unknown-linux-gnu>> Options: -g -O2 -ftime-report -c>>>> Before:>> reload CSE regs : 13.65 (81%) usr 0.01 (25%) sys 13.73>> (81%) wall 2812 kB ( 4%) ggc>> After:>> reload CSE regs : 0.16 ( 6%) usr 0.01 (14%) sys 0.17 ( 6%)>> wall 2812 kB ( 4%) ggc>>>>>>> It's a bit unfortunate, and probably avoidable, to create additional>>> garbage RTL. Maybe what's really needed is>>> df_set_flags (DF_DEFER_INSN_RESCAN);>>> at the top of postreload? Can you try that? Or, even simpler, avoid>>> the SET_REGNO macro, and check all other occurrences of it. There seem>>> to be a few, e.g. in caller-save.c or ira.c, which also probably>>> shouldn't invoke df.>>>> I updated the patch by introducing SET_REGNO_RAW and updated the>> postreload.c, caller-save.c and ira.c.>>>> I saw no new test failures.>> I was going to look at your patch, but you only included the SET_REGNO_RAW> changes. It's customary to repost the entire patch after you update it to> address issues from reviewers.
The included patch was the complete, as the comments I got simplified it.
I'm sorry if I missed something, and would be happy to try to address that.
Thanks,
Martin
>> jeff>