| Message ID | AANLkTi=njbubR5nWZvoheRbu0Htsvpz1vN6XO1GwMJyN@mail.gmail.com |
|---|---|
| State | New |
| Headers | show |
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 >
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