| Message ID | 558A0130.9010202@codesourcery.com |
|---|---|
| State | New |
| Headers | show |
On 24/06/15 02:00, Sandra Loosemore wrote: > On 06/18/2015 11:32 AM, Eric Botcazou wrote: >>> The attached patch teaches regrename to validate insns affected by each >>> register renaming before making the change. I can see at least two >>> other ways to handle this -- earlier, by rejecting renamings that result >>> in invalid instructions when it's searching for the best renaming; or >>> later, by validating the entire set of renamings as a group instead of >>> incrementally for each one -- but doing it all in regname_do_replace >>> seems least disruptive and risky in terms of the existing code. >> >> OK, but the patch looks incomplete, rename_chains should be adjusted >> as well, >> i.e. regrename_do_replace should now return a boolean. > > Like this? I tested this on nios2 and x86_64-linux-gnu, as before, plus > built for aarch64-linux-gnu and ran the gcc testsuite. Hopefully that was built with --with-cpu=cortex-a57 to enable the renaming pass ? Ramana > > The c6x back end also calls regrename_do_replace. I am not set up to > build or test on that target, and Bernd told me off-list that it would > never fail on that target anyway so I have left that code alone. > > -Sandra
On 06/24/2015 01:58 AM, Ramana Radhakrishnan wrote: > > > On 24/06/15 02:00, Sandra Loosemore wrote: >> On 06/18/2015 11:32 AM, Eric Botcazou wrote: >>>> The attached patch teaches regrename to validate insns affected by each >>>> register renaming before making the change. I can see at least two >>>> other ways to handle this -- earlier, by rejecting renamings that >>>> result >>>> in invalid instructions when it's searching for the best renaming; or >>>> later, by validating the entire set of renamings as a group instead of >>>> incrementally for each one -- but doing it all in regname_do_replace >>>> seems least disruptive and risky in terms of the existing code. >>> >>> OK, but the patch looks incomplete, rename_chains should be adjusted >>> as well, >>> i.e. regrename_do_replace should now return a boolean. >> >> Like this? I tested this on nios2 and x86_64-linux-gnu, as before, plus >> built for aarch64-linux-gnu and ran the gcc testsuite. > > Hopefully that was built with --with-cpu=cortex-a57 to enable the > renaming pass ? No, sorry. I was assuming there were compile-only unit tests for this pass that automatically add the right options to enable it. I don't know that I can actually run cortex-a57 code (I was struggling with a flaky test harness as it was). -Sandra
> Like this? I tested this on nios2 and x86_64-linux-gnu, as before, plus > built for aarch64-linux-gnu and ran the gcc testsuite. Yes, the patch is OK, modulo... > The c6x back end also calls regrename_do_replace. I am not set up to > build or test on that target, and Bernd told me off-list that it would > never fail on that target anyway so I have left that code alone. ... Bernd has obviously the final say here, but it would be better to add an assertion that it indeed did not fail (just build the cc1 as a sanity check). Thanks for adding the missing head comment to regrename_do_replace.
> Yes, the patch is OK, modulo...
But you also need the approval of an ARM maintainer.
On 06/23/2015 07:00 PM, Sandra Loosemore wrote: > On 06/18/2015 11:32 AM, Eric Botcazou wrote: >>> The attached patch teaches regrename to validate insns affected by each >>> register renaming before making the change. I can see at least two >>> other ways to handle this -- earlier, by rejecting renamings that result >>> in invalid instructions when it's searching for the best renaming; or >>> later, by validating the entire set of renamings as a group instead of >>> incrementally for each one -- but doing it all in regname_do_replace >>> seems least disruptive and risky in terms of the existing code. >> >> OK, but the patch looks incomplete, rename_chains should be adjusted >> as well, >> i.e. regrename_do_replace should now return a boolean. > > Like this? I tested this on nios2 and x86_64-linux-gnu, as before, plus > built for aarch64-linux-gnu and ran the gcc testsuite. > > The c6x back end also calls regrename_do_replace. I am not set up to > build or test on that target, and Bernd told me off-list that it would > never fail on that target anyway so I have left that code alone. > > -Sandra > > regrename-2.log > > > 2015-06-23 Chung-Lin Tang<cltang@codesourcery.com> > Sandra Loosemore<sandra@codesourcery.com> > > gcc/ > * regrename.h (regrename_do_replace): Change to return bool. > * regrename.c (rename_chains): Check return value of > regname_do_replace. > (regrename_do_replace): Re-validate the modified insns and > return bool status. > * config/aarch64/cortex-a57-fma-steering.c (rename_single_chain): > Update to match rename_chains changes. As Eric mentioned, please put an assert to verify that the call from the c6x backend never fails. The regrename and ARM bits are fine. Do you have a testcase that you can add to the suite? If so it'd be appreciated if you could include that too. Approved with the c6x assert if a testcase isn't available or exceedingly difficult to produce. jeff
On 06/25/2015 05:46 AM, Jeff Law wrote: > As Eric mentioned, please put an assert to verify that the call from the > c6x backend never fails. I'd be happiest if we had an assert on almost all targets. regrename has been designed in such a way that the replacements can't fail, if the constraints on the insns are correct. If there are ports which have borderline insns where that doesn't hold maybe we ought to have a target hook that says so. Bernd
> I'd be happiest if we had an assert on almost all targets. regrename has > been designed in such a way that the replacements can't fail, if the > constraints on the insns are correct. If there are ports which have > borderline insns where that doesn't hold maybe we ought to have a target > hook that says so. ARM both has "borderline" insns (in fact insns with match_parallel are enough) and benefits from the regrename pass so this doesn't seem to be really doable.
On 06/25/2015 03:53 PM, Eric Botcazou wrote: >> I'd be happiest if we had an assert on almost all targets. regrename has >> been designed in such a way that the replacements can't fail, if the >> constraints on the insns are correct. If there are ports which have >> borderline insns where that doesn't hold maybe we ought to have a target >> hook that says so. > > ARM both has "borderline" insns (in fact insns with match_parallel are enough) > and benefits from the regrename pass so this doesn't seem to be really doable. All I'm saying it would be nice to have a port say "validate regrename results", and use the assert only if the port doesn't request it. On most ports (excluding arm and nios2 by the sound of it) we'd still have an assert verifying that the regrename implementation is sound wrt doing the right thing with insn constraints. Bernd
Index: gcc/regrename.h =================================================================== --- gcc/regrename.h (revision 224700) +++ gcc/regrename.h (working copy) @@ -91,6 +91,6 @@ extern void regrename_analyze (bitmap); extern du_head_p regrename_chain_from_id (unsigned int); extern int find_rename_reg (du_head_p, enum reg_class, HARD_REG_SET *, int, bool); -extern void regrename_do_replace (du_head_p, int); +extern bool regrename_do_replace (du_head_p, int); #endif Index: gcc/regrename.c =================================================================== --- gcc/regrename.c (revision 224700) +++ gcc/regrename.c (working copy) @@ -496,12 +496,20 @@ rename_chains (void) continue; } - if (dump_file) - fprintf (dump_file, ", renamed as %s\n", reg_names[best_new_reg]); - - regrename_do_replace (this_head, best_new_reg); - tick[best_new_reg] = ++this_tick; - df_set_regs_ever_live (best_new_reg, true); + if (regrename_do_replace (this_head, best_new_reg)) + { + if (dump_file) + fprintf (dump_file, ", renamed as %s\n", reg_names[best_new_reg]); + tick[best_new_reg] = ++this_tick; + df_set_regs_ever_live (best_new_reg, true); + } + else + { + if (dump_file) + fprintf (dump_file, ", renaming as %s failed\n", + reg_names[best_new_reg]); + tick[reg] = ++this_tick; + } } } @@ -927,7 +935,13 @@ regrename_analyze (bitmap bb_mask) bb->aux = NULL; } -void +/* Attempt to replace all uses of the register in the chain beginning with + HEAD with REG. Returns true on success and false if the replacement is + rejected because the insns would not validate. The latter can happen + e.g. if a match_parallel predicate enforces restrictions on register + numbering in its subpatterns. */ + +bool regrename_do_replace (struct du_head *head, int reg) { struct du_chain *chain; @@ -941,22 +955,26 @@ regrename_do_replace (struct du_head *he int reg_ptr = REG_POINTER (*chain->loc); if (DEBUG_INSN_P (chain->insn) && REGNO (*chain->loc) != base_regno) - INSN_VAR_LOCATION_LOC (chain->insn) = gen_rtx_UNKNOWN_VAR_LOC (); + validate_change (chain->insn, &(INSN_VAR_LOCATION_LOC (chain->insn)), + gen_rtx_UNKNOWN_VAR_LOC (), true); else { - *chain->loc = gen_raw_REG (GET_MODE (*chain->loc), reg); + validate_change (chain->insn, chain->loc, + gen_raw_REG (GET_MODE (*chain->loc), reg), true); if (regno >= FIRST_PSEUDO_REGISTER) ORIGINAL_REGNO (*chain->loc) = regno; REG_ATTRS (*chain->loc) = attr; REG_POINTER (*chain->loc) = reg_ptr; } - - df_insn_rescan (chain->insn); } + if (!apply_change_group ()) + return false; + mode = GET_MODE (*head->first->loc); head->regno = reg; head->nregs = hard_regno_nregs[reg][mode]; + return true; } Index: gcc/config/aarch64/cortex-a57-fma-steering.c =================================================================== --- gcc/config/aarch64/cortex-a57-fma-steering.c (revision 224700) +++ gcc/config/aarch64/cortex-a57-fma-steering.c (working copy) @@ -288,11 +288,19 @@ rename_single_chain (du_head_p head, HAR return false; } - if (dump_file) - fprintf (dump_file, ", renamed as %s\n", reg_names[best_new_reg]); - - regrename_do_replace (head, best_new_reg); - df_set_regs_ever_live (best_new_reg, true); + if (regrename_do_replace (head, best_new_reg)) + { + if (dump_file) + fprintf (dump_file, ", renamed as %s\n", reg_names[best_new_reg]); + df_set_regs_ever_live (best_new_reg, true); + } + else + { + if (dump_file) + fprintf (dump_file, ", renaming as %s failed\n", + reg_names[best_new_reg]); + return false; + } return true; }