Patchwork Update postreload.c to avoid long compilation time

login
register
mail settings
Submitter Martin Thuresson
Date Aug. 4, 2010, 6:39 p.m.
Message ID <AANLkTi=njbubR5nWZvoheRbu0Htsvpz1vN6XO1GwMJyN@mail.gmail.com>
Download mbox | patch
Permalink /patch/60881/
State New
Headers show

Comments

Martin Thuresson - Aug. 4, 2010, 6:39 p.m.
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
Bernd Schmidt - Aug. 5, 2010, 8:08 p.m.
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
Jeff Law - Aug. 5, 2010, 9:14 p.m.
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
Martin Thuresson - Aug. 5, 2010, 10:11 p.m.
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
>

Patch

Index: gcc/postreload.c
===================================================================
--- gcc/postreload.c	(revision 162726)
+++ gcc/postreload.c	(working copy)
@@ -528,7 +528,7 @@  reload_cse_simplify_operands (rtx insn, 
 	  if (! TEST_HARD_REG_BIT (equiv_regs[i], regno))
 	    continue;
 
-	  SET_REGNO (testreg, regno);
+	  SET_REGNO_RAW (testreg, regno);
 	  PUT_MODE (testreg, mode);
 
 	  /* We found a register equal to this operand.  Now look for all
Index: gcc/caller-save.c
===================================================================
--- gcc/caller-save.c	(revision 162726)
+++ gcc/caller-save.c	(working copy)
@@ -124,7 +124,7 @@  reg_save_code (int reg, enum machine_mod
 
   /* Update the register number and modes of the register
      and memory operand.  */
-  SET_REGNO (test_reg, reg);
+  SET_REGNO_RAW (test_reg, reg);
   PUT_MODE (test_reg, mode);
   PUT_MODE (test_mem, mode);
 
Index: gcc/ira.c
===================================================================
--- gcc/ira.c	(revision 162726)
+++ gcc/ira.c	(working copy)
@@ -1219,9 +1219,9 @@  setup_prohibited_mode_move_regs (void)
 	{
 	  if (! HARD_REGNO_MODE_OK (j, (enum machine_mode) i))
 	    continue;
-	  SET_REGNO (test_reg1, j);
+	  SET_REGNO_RAW (test_reg1, j);
 	  PUT_MODE (test_reg1, (enum machine_mode) i);
-	  SET_REGNO (test_reg2, j);
+	  SET_REGNO_RAW (test_reg2, j);
 	  PUT_MODE (test_reg2, (enum machine_mode) i);
 	  INSN_CODE (move_insn) = -1;
 	  recog_memoized (move_insn);
Index: gcc/rtl.h
===================================================================
--- gcc/rtl.h	(revision 162726)
+++ gcc/rtl.h	(working copy)
@@ -1039,6 +1039,7 @@  enum label_kind
    be used on RHS.  Use SET_REGNO to change the value.  */
 #define REGNO(RTX) (rhs_regno(RTX))
 #define SET_REGNO(RTX,N) (df_ref_change_reg_with_loc (REGNO(RTX), N, RTX), XCUINT (RTX, 0, REG) = N)
+#define SET_REGNO_RAW(RTX,N) (XCUINT (RTX, 0, REG) = N)
 
 /* ORIGINAL_REGNO holds the number the register originally had; for a
    pseudo register turned into a hard reg this will hold the old pseudo