Message ID | CABu31nO-FyF_T3KCv1PW-ndAotCKdxpjzROm7kjTEb5p2DEt3A@mail.gmail.com |
---|---|
State | New |
Headers | show |
Hi Steven, > From: Steven Bosscher [mailto:stevenb.gcc@gmail.com] > Sent: Friday, March 20, 2015 3:54 PM > > > What I meant, is that I believe the tests are already done in > hash_scan_set and should be redundant in cprop_insn (i.e. the test can > be replaced with gcc_[checking_]assert). Ok. > > I've attached a patch with some changes to it: introduce cprop_reg_p() > to get rid of all the "REG_P && regno > FIRST_PSEUDO_REGISTER" tests. > I still have the cprop_constant_p and cprop_reg_p tests in cprop_insn > but this weekend I'll try with gcc_checking_asserts instead. Please > have a look at the patch and let me know if you like it (given it's > mostly yours I hope you do like it ;-) I think it would be preferable to introduce PSEUDO_REG_P in rtl.h as this seems like a common pattern enough [1]. It would be nice to have a HARD_REG_P that would be cover the other common patterns REG_P && < FIRST_PSEUDO_REGISTER and REG_P && HARD_REGISTER_P but I can't come up with a good name (HARD_REGISTER_P is confusing because it doesn't check if it's a register in the first place). I noticed in do_local_cprop you replace >= FIRST_PSEUDO_REGISTER by cprop_reg_p without removing the REG_P as well. In implicit_set_cond_p there is a replacement of !REG_P || HARD_REGISTER_P by cprop_reg_p. It seems to me it should rather be replaced by !cprop_reg_p. Otherwise it looks ok. [1] grep -R "REG_P .*&&.*>= FIRST_PSEUDO_REGISTER" . | wc -l returns 23 > > Bootstrapped & tested on powerpc64-unknown-linux-gnu. In building all > of cc1, 35 extra copies are propagated with the patch. I'll try to launch a build and testsuite run with these 2 issues fixed before I leave tonight and will report the result on Monday. Best regards, Thomas
> From: gcc-patches-owner@gcc.gnu.org [mailto:gcc-patches- > owner@gcc.gnu.org] On Behalf Of Thomas Preud'homme > > I noticed in do_local_cprop you replace >= FIRST_PSEUDO_REGISTER by > cprop_reg_p without removing the REG_P as well. Sorry, I missed the parenthesis. REG_P needs indeed to be kept. I'd be tempted to use !HARD_REGISTER_P instead since REG_P is already checked but I don't mind either way. Best regards, Thomas
On Fri, Mar 20, 2015 at 11:27 AM, Thomas Preud'homme wrote: > Sorry, I missed the parenthesis. REG_P needs indeed to be kept. I'd be > tempted to use !HARD_REGISTER_P instead since REG_P is already > checked but I don't mind either way. I put the cprop_reg_p check there instead of !HARD_REGISTER_P because I like to be able to quickly find all places where a similar check is performed. The check is whether the reg is something that copy propagation can handle, and that is what I added cprop_reg_p for. (Note that cprop can _currently_ handle only pseudos but there is no reason why a limited set of hard regs can't be handled also, e.g. the flag registers like in targetm.fixed_condition_code_regs). In this case, the result is that REG_P is checked twice. But then again, cprop_reg_p will be inlined and the double check optimized away. Anyway, I guess we've bikeshedded long enough over this patch as it is :-) Let's post a final form and declare it OK for stage1. As for PSEUDO_REG_P: If it were up to me, I'd like to have in rtl.h: static bool hard_register_p (rtx x) { return (REG_P (x) && HARD_REGISTER_NUM_P (REGNO (x))); } static bool pseudo_register_p (rtx x) { return (REG_P (x) && !HARD_REGISTER_NUM_P (REGNO (x))); } and do away with all the FIRST_PSEUDO_REGISTER tests. But I've proposed this in the past and there was opposition. Perhaps when we introduce a rtx_reg class... Ciao! Steven
> From: Steven Bosscher [mailto:stevenb.gcc@gmail.com] > Sent: Friday, March 20, 2015 8:14 PM > > I put the cprop_reg_p check there instead of !HARD_REGISTER_P > because > I like to be able to quickly find all places where a similar check is > performed. The check is whether the reg is something that copy > propagation can handle, and that is what I added cprop_reg_p for. Makes sense indeed. I didn't think about the meaning of it. > (Note that cprop can _currently_ handle only pseudos but there is no > reason why a limited set of hard regs can't be handled also, e.g. the > flag registers like in targetm.fixed_condition_code_regs). > > In this case, the result is that REG_P is checked twice. > But then again, cprop_reg_p will be inlined and the double check > optimized away. True. > > Anyway, I guess we've bikeshedded long enough over this patch as it is > :-) Let's post a final form and declare it OK for stage1. What about the cprop_reg_p that needs to be negated? Did I miss something that makes it ok? > > As for PSEUDO_REG_P: If it were up to me, I'd like to have in rtl.h: > > static bool > hard_register_p (rtx x) > { > return (REG_P (x) && HARD_REGISTER_NUM_P (REGNO (x))); > } > > static bool > pseudo_register_p (rtx x) > { > return (REG_P (x) && !HARD_REGISTER_NUM_P (REGNO (x))); > } > > and do away with all the FIRST_PSEUDO_REGISTER tests. But I've > proposed this in the past and there was opposition. Perhaps when we > introduce a rtx_reg class... Ok I'll try to dig up what was the reasons presented. Anyway, it would be done in a separate patch so not a problem for this one. FYI testing your patch with the one cprop_reg_p negated as said in my previous email shows no regression on arm-none-eabi cross-compiler targeting Cortex-M3. Testing for x86_64 is ongoing. Best regards, Thomas
On Mon, Mar 23, 2015 at 12:01 PM, Thomas Preud'homme wrote: > What about the cprop_reg_p that needs to be negated? Did I miss something > that makes it ok? You didn't miss anything. I sent the wrong patch. The one I tested on ppc64 also has the condition reversed: @@ -1328,9 +1329,8 @@ implicit_set_cond_p (const_rtx cond) if (GET_CODE (cond) != EQ && GET_CODE (cond) != NE) return false; - /* The first operand of COND must be a pseudo-reg. */ - if (! REG_P (XEXP (cond, 0)) - || HARD_REGISTER_P (XEXP (cond, 0))) + /* The first operand of COND must be a register we can propagate. */ + if (! cprop_reg_p (XEXP (cond, 0))) return false; /* The second operand of COND must be a suitable constant. */
> From: gcc-patches-owner@gcc.gnu.org [mailto:gcc-patches- > owner@gcc.gnu.org] On Behalf Of Thomas Preud'homme > > FYI testing your patch with the one cprop_reg_p negated as said in my > previous email shows no regression on arm-none-eabi cross-compiler > targeting Cortex-M3. Testing for x86_64 is ongoing. Sorry, I forgot to report back on this. No regression as well on x86_64-linux-gnu. Do you want me to respin the patch (adding the testcase from the patch I sent, fixing the indentation and adding a ChangeLog)? Best regards, Thomas
Index: cprop.c =================================================================== --- cprop.c (revision 221523) +++ cprop.c (working copy) @@ -285,6 +285,15 @@ cprop_constant_p (const_rtx x) return CONSTANT_P (x) && (GET_CODE (x) != CONST || shared_const_p (x)); } +/* Determine whether the rtx X should be treated as a register that can + be propagated. Any pseudo-register is fine. */ + +static bool +cprop_reg_p (const_rtx x) +{ + return REG_P (x) && !HARD_REGISTER_P (x); +} + /* Scan SET present in INSN and add an entry to the hash TABLE. IMPLICIT is true if it's an implicit set, false otherwise. */ @@ -295,8 +304,7 @@ hash_scan_set (rtx set, rtx_insn *insn, struct has rtx src = SET_SRC (set); rtx dest = SET_DEST (set); - if (REG_P (dest) - && ! HARD_REGISTER_P (dest) + if (cprop_reg_p (dest) && reg_available_p (dest, insn) && can_copy_p (GET_MODE (dest))) { @@ -321,9 +329,8 @@ hash_scan_set (rtx set, rtx_insn *insn, struct has src = XEXP (note, 0), set = gen_rtx_SET (VOIDmode, dest, src); /* Record sets for constant/copy propagation. */ - if ((REG_P (src) + if ((cprop_reg_p (src) && src != dest - && ! HARD_REGISTER_P (src) && reg_available_p (src, insn)) || cprop_constant_p (src)) insert_set_in_table (dest, src, insn, table, implicit); @@ -821,15 +828,15 @@ try_replace_reg (rtx from, rtx to, rtx_insn *insn) return success; } -/* Find a set of REGNOs that are available on entry to INSN's block. Return - NULL no such set is found. */ +/* Find a set of REGNOs that are available on entry to INSN's block. If found, + SET_RET[0] will be assigned a set with a register source and SET_RET[1] a + set with a constant source. If not found the corresponding entry is set to + NULL. */ -static struct cprop_expr * -find_avail_set (int regno, rtx_insn *insn) +static void +find_avail_set (int regno, rtx_insn *insn, struct cprop_expr *set_ret[2]) { - /* SET1 contains the last set found that can be returned to the caller for - use in a substitution. */ - struct cprop_expr *set1 = 0; + set_ret[0] = set_ret[1] = NULL; /* Loops are not possible here. To get a loop we would need two sets available at the start of the block containing INSN. i.e. we would @@ -869,8 +876,10 @@ try_replace_reg (rtx from, rtx to, rtx_insn *insn) If the source operand changed, we may still use it for the next iteration of this loop, but we may not use it for substitutions. */ - if (cprop_constant_p (src) || reg_not_set_p (src, insn)) - set1 = set; + if (cprop_constant_p (src)) + set_ret[1] = set; + else if (reg_not_set_p (src, insn)) + set_ret[0] = set; /* If the source of the set is anything except a register, then we have reached the end of the copy chain. */ @@ -881,10 +890,6 @@ try_replace_reg (rtx from, rtx to, rtx_insn *insn) and see if we have an available copy into SRC. */ regno = REGNO (src); } - - /* SET1 holds the last set that was available and anticipatable at - INSN. */ - return set1; } /* Subroutine of cprop_insn that tries to propagate constants into @@ -1050,7 +1055,8 @@ cprop_insn (rtx_insn *insn) int changed = 0, changed_this_round; rtx note; -retry: + do + { changed_this_round = 0; reg_use_count = 0; note_uses (&PATTERN (insn), find_used_regs, NULL); @@ -1064,8 +1070,8 @@ cprop_insn (rtx_insn *insn) { rtx reg_used = reg_use_table[i]; unsigned int regno = REGNO (reg_used); - rtx src; - struct cprop_expr *set; + rtx src_cst = NULL, src_reg = NULL; + struct cprop_expr *set[2]; /* If the register has already been set in this block, there's nothing we can do. */ @@ -1074,17 +1080,16 @@ cprop_insn (rtx_insn *insn) /* Find an assignment that sets reg_used and is available at the start of the block. */ - set = find_avail_set (regno, insn); - if (! set) - continue; + find_avail_set (regno, insn, set); + if (set[0]) + src_reg = set[0]->src; + if (set[1]) + src_cst = set[1]->src; - src = set->src; - /* Constant propagation. */ - if (cprop_constant_p (src)) + if (src_cst && cprop_constant_p (src_cst) + && constprop_register (reg_used, src_cst, insn)) { - if (constprop_register (reg_used, src, insn)) - { changed_this_round = changed = 1; global_const_prop_count++; if (dump_file != NULL) @@ -1093,19 +1098,17 @@ cprop_insn (rtx_insn *insn) "GLOBAL CONST-PROP: Replacing reg %d in ", regno); fprintf (dump_file, "insn %d with constant ", INSN_UID (insn)); - print_rtl (dump_file, src); + print_rtl (dump_file, src_cst); fprintf (dump_file, "\n"); } if (insn->deleted ()) return 1; } - } - else if (REG_P (src) - && REGNO (src) >= FIRST_PSEUDO_REGISTER - && REGNO (src) != regno) + /* Copy propagation. */ + else if (src_reg && cprop_reg_p (src_reg) + && REGNO (src_reg) != regno + && try_replace_reg (reg_used, src_reg, insn)) { - if (try_replace_reg (reg_used, src, insn)) - { changed_this_round = changed = 1; global_copy_prop_count++; if (dump_file != NULL) @@ -1113,7 +1116,7 @@ cprop_insn (rtx_insn *insn) fprintf (dump_file, "GLOBAL COPY-PROP: Replacing reg %d in insn %d", regno, INSN_UID (insn)); - fprintf (dump_file, " with reg %d\n", REGNO (src)); + fprintf (dump_file, " with reg %d\n", REGNO (src_reg)); } /* The original insn setting reg_used may or may not now be @@ -1123,12 +1126,10 @@ cprop_insn (rtx_insn *insn) and made things worse. */ } } - + } /* If try_replace_reg simplified the insn, the regs found by find_used_regs may not be valid anymore. Start over. */ - if (changed_this_round) - goto retry; - } + while (changed_this_round); if (changed && DEBUG_INSN_P (insn)) return 0; @@ -1191,7 +1192,7 @@ do_local_cprop (rtx x, rtx_insn *insn) /* Rule out USE instructions and ASM statements as we don't want to change the hard registers mentioned. */ if (REG_P (x) - && (REGNO (x) >= FIRST_PSEUDO_REGISTER + && (cprop_reg_p (x) || (GET_CODE (PATTERN (insn)) != USE && asm_noperands (PATTERN (insn)) < 0))) { @@ -1207,7 +1208,7 @@ do_local_cprop (rtx x, rtx_insn *insn) if (cprop_constant_p (this_rtx)) newcnst = this_rtx; - if (REG_P (this_rtx) && REGNO (this_rtx) >= FIRST_PSEUDO_REGISTER + if (cprop_reg_p (this_rtx) /* Don't copy propagate if it has attached REG_EQUIV note. At this point this only function parameters should have REG_EQUIV notes and if the argument slot is used somewhere @@ -1328,9 +1329,8 @@ implicit_set_cond_p (const_rtx cond) if (GET_CODE (cond) != EQ && GET_CODE (cond) != NE) return false; - /* The first operand of COND must be a pseudo-reg. */ - if (! REG_P (XEXP (cond, 0)) - || HARD_REGISTER_P (XEXP (cond, 0))) + /* The first operand of COND must be a register we can propagate. */ + if (cprop_reg_p (XEXP (cond, 0))) return false; /* The second operand of COND must be a suitable constant. */