* [patch] fix regrename pass to ensure renamings produce valid insns @ 2015-06-17 17:36 Sandra Loosemore 2015-06-17 19:23 ` Richard Sandiford ` (2 more replies) 0 siblings, 3 replies; 25+ messages in thread From: Sandra Loosemore @ 2015-06-17 17:36 UTC (permalink / raw) To: GCC Patches; +Cc: Chung-Lin Tang [-- Attachment #1: Type: text/plain, Size: 1446 bytes --] We ran across problems with the regrename pass introducing invalid insns while working on support for new load/store multiple instructions on nios2. We're using an implementation similar to what ARM already does, with a match_parallel predicate testing for restrictions on the underlying machine instructions that can't be expressed using GCC register constraints. (E.g., the register numbers do not have to be sequential, but they do have to be ascending/descending and a combination that can be encoded in the machine instruction.) 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. There are a couple of old bugzilla tickets for instruction-doesn't-satisfy-its-constraints ICEs in regrename that might or might not be related to this. The symptom we saw was not an ICE, just bad code being emitted that resulted in assembler errors. I've bootstrapped and regression-tested this on x86_64-linux-gnu in addition to our testing with the pending nios2 patch set. OK to commit? -Sandra [-- Attachment #2: regrename.log --] [-- Type: text/x-log, Size: 180 bytes --] 2015-06-17 Chung-Lin Tang <cltang@codesourcery.com> Sandra Loosemore <sandra@codesourcery.com> gcc/ * regrename.c (regrename_do_replace): Re-validate the modified insns. [-- Attachment #3: regrename.patch --] [-- Type: text/x-patch, Size: 1081 bytes --] Index: gcc/regrename.c =================================================================== --- gcc/regrename.c (revision 224532) +++ gcc/regrename.c (working copy) @@ -942,19 +942,22 @@ 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; + mode = GET_MODE (*head->first->loc); head->regno = reg; head->nregs = hard_regno_nregs[reg][mode]; ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [patch] fix regrename pass to ensure renamings produce valid insns 2015-06-17 17:36 [patch] fix regrename pass to ensure renamings produce valid insns Sandra Loosemore @ 2015-06-17 19:23 ` Richard Sandiford 2015-06-18 18:26 ` Eric Botcazou 2015-11-06 10:48 ` Bernd Schmidt 2 siblings, 0 replies; 25+ messages in thread From: Richard Sandiford @ 2015-06-17 19:23 UTC (permalink / raw) To: Sandra Loosemore; +Cc: GCC Patches, Chung-Lin Tang Sandra Loosemore <sandra@codesourcery.com> writes: > Index: gcc/regrename.c > =================================================================== > --- gcc/regrename.c (revision 224532) > +++ gcc/regrename.c (working copy) > @@ -942,19 +942,22 @@ 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; > } I think it'd be cleaner to do the adjustments on the new register before passing it to validate_change. LGTM otherwise for what it's worth. Thanks, Richard ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [patch] fix regrename pass to ensure renamings produce valid insns 2015-06-17 17:36 [patch] fix regrename pass to ensure renamings produce valid insns Sandra Loosemore 2015-06-17 19:23 ` Richard Sandiford @ 2015-06-18 18:26 ` Eric Botcazou 2015-06-24 3:31 ` Sandra Loosemore 2015-11-06 10:48 ` Bernd Schmidt 2 siblings, 1 reply; 25+ messages in thread From: Eric Botcazou @ 2015-06-18 18:26 UTC (permalink / raw) To: Sandra Loosemore; +Cc: gcc-patches, Chung-Lin Tang > 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. -- Eric Botcazou ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [patch] fix regrename pass to ensure renamings produce valid insns 2015-06-18 18:26 ` Eric Botcazou @ 2015-06-24 3:31 ` Sandra Loosemore 2015-06-24 7:59 ` Ramana Radhakrishnan ` (2 more replies) 0 siblings, 3 replies; 25+ messages in thread From: Sandra Loosemore @ 2015-06-24 3:31 UTC (permalink / raw) To: Eric Botcazou Cc: gcc-patches, Chung-Lin Tang, Marcus Shawcroft, Richard Earnshaw, Schmidt, Bernd - Code Sourcery [-- Attachment #1: Type: text/plain, Size: 1040 bytes --] 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 [-- Attachment #2: regrename-2.log --] [-- Type: text/x-log, Size: 433 bytes --] 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. [-- Attachment #3: regrename-2.patch --] [-- Type: text/x-patch, Size: 3732 bytes --] 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; } ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [patch] fix regrename pass to ensure renamings produce valid insns 2015-06-24 3:31 ` Sandra Loosemore @ 2015-06-24 7:59 ` Ramana Radhakrishnan 2015-06-24 14:57 ` Sandra Loosemore 2015-06-24 16:50 ` Eric Botcazou 2015-06-25 3:53 ` Jeff Law 2 siblings, 1 reply; 25+ messages in thread From: Ramana Radhakrishnan @ 2015-06-24 7:59 UTC (permalink / raw) To: Sandra Loosemore, Eric Botcazou Cc: gcc-patches, Chung-Lin Tang, Marcus Shawcroft, Richard Earnshaw, Schmidt, Bernd - Code Sourcery 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 ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [patch] fix regrename pass to ensure renamings produce valid insns 2015-06-24 7:59 ` Ramana Radhakrishnan @ 2015-06-24 14:57 ` Sandra Loosemore 0 siblings, 0 replies; 25+ messages in thread From: Sandra Loosemore @ 2015-06-24 14:57 UTC (permalink / raw) To: Ramana Radhakrishnan Cc: Eric Botcazou, gcc-patches, Chung-Lin Tang, Marcus Shawcroft, Richard Earnshaw, Schmidt, Bernd - Code Sourcery 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 ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [patch] fix regrename pass to ensure renamings produce valid insns 2015-06-24 3:31 ` Sandra Loosemore 2015-06-24 7:59 ` Ramana Radhakrishnan @ 2015-06-24 16:50 ` Eric Botcazou 2015-06-24 16:59 ` Eric Botcazou 2015-06-25 3:53 ` Jeff Law 2 siblings, 1 reply; 25+ messages in thread From: Eric Botcazou @ 2015-06-24 16:50 UTC (permalink / raw) To: Sandra Loosemore Cc: gcc-patches, Chung-Lin Tang, Marcus Shawcroft, Richard Earnshaw, Schmidt, Bernd - Code Sourcery > 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. -- Eric Botcazou ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [patch] fix regrename pass to ensure renamings produce valid insns 2015-06-24 16:50 ` Eric Botcazou @ 2015-06-24 16:59 ` Eric Botcazou 0 siblings, 0 replies; 25+ messages in thread From: Eric Botcazou @ 2015-06-24 16:59 UTC (permalink / raw) To: Sandra Loosemore Cc: gcc-patches, Chung-Lin Tang, Marcus Shawcroft, Richard Earnshaw, Schmidt, Bernd - Code Sourcery > Yes, the patch is OK, modulo... But you also need the approval of an ARM maintainer. -- Eric Botcazou ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [patch] fix regrename pass to ensure renamings produce valid insns 2015-06-24 3:31 ` Sandra Loosemore 2015-06-24 7:59 ` Ramana Radhakrishnan 2015-06-24 16:50 ` Eric Botcazou @ 2015-06-25 3:53 ` Jeff Law 2015-06-25 12:30 ` Bernd Schmidt 2015-06-29 0:18 ` Sandra Loosemore 2 siblings, 2 replies; 25+ messages in thread From: Jeff Law @ 2015-06-25 3:53 UTC (permalink / raw) To: Sandra Loosemore, Eric Botcazou Cc: gcc-patches, Chung-Lin Tang, Marcus Shawcroft, Richard Earnshaw, Schmidt, Bernd - Code Sourcery 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 ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [patch] fix regrename pass to ensure renamings produce valid insns 2015-06-25 3:53 ` Jeff Law @ 2015-06-25 12:30 ` Bernd Schmidt 2015-06-25 13:54 ` Eric Botcazou 2015-06-29 0:18 ` Sandra Loosemore 1 sibling, 1 reply; 25+ messages in thread From: Bernd Schmidt @ 2015-06-25 12:30 UTC (permalink / raw) To: Jeff Law, Sandra Loosemore, Eric Botcazou Cc: gcc-patches, Chung-Lin Tang, Marcus Shawcroft, Richard Earnshaw 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 ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [patch] fix regrename pass to ensure renamings produce valid insns 2015-06-25 12:30 ` Bernd Schmidt @ 2015-06-25 13:54 ` Eric Botcazou 2015-06-25 13:59 ` Bernd Schmidt 0 siblings, 1 reply; 25+ messages in thread From: Eric Botcazou @ 2015-06-25 13:54 UTC (permalink / raw) To: Bernd Schmidt Cc: gcc-patches, Jeff Law, Sandra Loosemore, Chung-Lin Tang, Marcus Shawcroft, Richard Earnshaw > 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. -- Eric Botcazou ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [patch] fix regrename pass to ensure renamings produce valid insns 2015-06-25 13:54 ` Eric Botcazou @ 2015-06-25 13:59 ` Bernd Schmidt 0 siblings, 0 replies; 25+ messages in thread From: Bernd Schmidt @ 2015-06-25 13:59 UTC (permalink / raw) To: Eric Botcazou Cc: gcc-patches, Jeff Law, Sandra Loosemore, Chung-Lin Tang, Marcus Shawcroft, Richard Earnshaw 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 ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [patch] fix regrename pass to ensure renamings produce valid insns 2015-06-25 3:53 ` Jeff Law 2015-06-25 12:30 ` Bernd Schmidt @ 2015-06-29 0:18 ` Sandra Loosemore 2015-06-30 3:54 ` Kito Cheng 2015-07-01 17:03 ` Jeff Law 1 sibling, 2 replies; 25+ messages in thread From: Sandra Loosemore @ 2015-06-29 0:18 UTC (permalink / raw) To: Jeff Law Cc: Eric Botcazou, gcc-patches, Chung-Lin Tang, Marcus Shawcroft, Richard Earnshaw, Schmidt, Bernd - Code Sourcery [-- Attachment #1: Type: text/plain, Size: 2716 bytes --] On 06/24/2015 09:46 PM, Jeff Law wrote: > 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. Thanks. I've committed the attached version. Re the testcase, this fixed 16 FAILs on existing tests in the gcc testsuite with the forthcoming nios2 load/store multiple instruction support, all assembler errors due to the bad instructions being generated. There's nothing I can do on nios2 for a testcase until I get those patches committed (I'm still trying to re-test and tidy them up for submission), plus I think the failures are rather fragile -- depending on the register allocator choosing an initial register numbering that allows peephole optimizers to trigger, etc. But, I will revisit this later and see what I can do. -Sandra [-- Attachment #2: regrename-3.log --] [-- Type: text/x-log, Size: 524 bytes --] 2015-06-28 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. * config/c6x/c6x.c (try_rename_operands): Assert that regrename_do_replace returns true. [-- Attachment #3: regrename-3.patch --] [-- Type: text/x-patch, Size: 4639 bytes --] Index: gcc/regrename.c =================================================================== --- gcc/regrename.c (revision 225104) +++ 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/regrename.h =================================================================== --- gcc/regrename.h (revision 225104) +++ 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/config/aarch64/cortex-a57-fma-steering.c =================================================================== --- gcc/config/aarch64/cortex-a57-fma-steering.c (revision 225104) +++ gcc/config/aarch64/cortex-a57-fma-steering.c (working copy) @@ -289,11 +289,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; } Index: gcc/config/c6x/c6x.c =================================================================== --- gcc/config/c6x/c6x.c (revision 225104) +++ gcc/config/c6x/c6x.c (working copy) @@ -3516,7 +3516,7 @@ try_rename_operands (rtx_insn *head, rtx best_reg = find_rename_reg (this_head, super_class, &unavailable, old_reg, true); - regrename_do_replace (this_head, best_reg); + gcc_assert (regrename_do_replace (this_head, best_reg)); count_unit_reqs (new_reqs, head, PREV_INSN (tail)); merge_unit_reqs (new_reqs); @@ -3529,7 +3529,7 @@ try_rename_operands (rtx_insn *head, rtx unit_req_imbalance (reqs), unit_req_imbalance (new_reqs)); } if (unit_req_imbalance (new_reqs) > unit_req_imbalance (reqs)) - regrename_do_replace (this_head, old_reg); + gcc_assert (regrename_do_replace (this_head, old_reg)); else memcpy (reqs, new_reqs, sizeof (unit_req_table)); ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [patch] fix regrename pass to ensure renamings produce valid insns 2015-06-29 0:18 ` Sandra Loosemore @ 2015-06-30 3:54 ` Kito Cheng 2015-06-30 5:08 ` Sandra Loosemore 2015-07-01 17:03 ` Jeff Law 1 sibling, 1 reply; 25+ messages in thread From: Kito Cheng @ 2015-06-30 3:54 UTC (permalink / raw) To: Sandra Loosemore Cc: Jeff Law, Eric Botcazou, gcc-patches, Chung-Lin Tang, Marcus Shawcroft, Richard Earnshaw, Schmidt, Bernd - Code Sourcery Hi all: This patch seem will broken when disable assert checking for c6x.... Index: gcc/config/c6x/c6x.c =================================================================== --- gcc/config/c6x/c6x.c (revision 225104) +++ gcc/config/c6x/c6x.c (working copy) @@ -3516,7 +3516,7 @@ try_rename_operands (rtx_insn *head, rtx best_reg = find_rename_reg (this_head, super_class, &unavailable, old_reg, true); - regrename_do_replace (this_head, best_reg); + gcc_assert (regrename_do_replace (this_head, best_reg)); count_unit_reqs (new_reqs, head, PREV_INSN (tail)); merge_unit_reqs (new_reqs); @@ -3529,7 +3529,7 @@ try_rename_operands (rtx_insn *head, rtx unit_req_imbalance (reqs), unit_req_imbalance (new_reqs)); } if (unit_req_imbalance (new_reqs) > unit_req_imbalance (reqs)) - regrename_do_replace (this_head, old_reg); + gcc_assert (regrename_do_replace (this_head, old_reg)); else memcpy (reqs, new_reqs, sizeof (unit_req_table)); On Mon, Jun 29, 2015 at 5:08 AM, Sandra Loosemore <sandra@codesourcery.com> wrote: > On 06/24/2015 09:46 PM, Jeff Law wrote: >> >> 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. > > > Thanks. I've committed the attached version. > > Re the testcase, this fixed 16 FAILs on existing tests in the gcc testsuite > with the forthcoming nios2 load/store multiple instruction support, all > assembler errors due to the bad instructions being generated. There's > nothing I can do on nios2 for a testcase until I get those patches committed > (I'm still trying to re-test and tidy them up for submission), plus I think > the failures are rather fragile -- depending on the register allocator > choosing an initial register numbering that allows peephole optimizers to > trigger, etc. But, I will revisit this later and see what I can do. > > -Sandra > ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [patch] fix regrename pass to ensure renamings produce valid insns 2015-06-30 3:54 ` Kito Cheng @ 2015-06-30 5:08 ` Sandra Loosemore 2015-06-30 5:26 ` Chung-Lin Tang 0 siblings, 1 reply; 25+ messages in thread From: Sandra Loosemore @ 2015-06-30 5:08 UTC (permalink / raw) To: Kito Cheng Cc: Jeff Law, Eric Botcazou, gcc-patches, Chung-Lin Tang, Marcus Shawcroft, Richard Earnshaw, Schmidt, Bernd - Code Sourcery On 06/29/2015 09:07 PM, Kito Cheng wrote: > Hi all: > > This patch seem will broken when disable assert checking for c6x.... > > Index: gcc/config/c6x/c6x.c > =================================================================== > --- gcc/config/c6x/c6x.c (revision 225104) > +++ gcc/config/c6x/c6x.c (working copy) > @@ -3516,7 +3516,7 @@ try_rename_operands (rtx_insn *head, rtx > best_reg = > find_rename_reg (this_head, super_class, &unavailable, old_reg, true); > > - regrename_do_replace (this_head, best_reg); > + gcc_assert (regrename_do_replace (this_head, best_reg)); > > count_unit_reqs (new_reqs, head, PREV_INSN (tail)); > merge_unit_reqs (new_reqs); > @@ -3529,7 +3529,7 @@ try_rename_operands (rtx_insn *head, rtx > unit_req_imbalance (reqs), unit_req_imbalance (new_reqs)); > } > if (unit_req_imbalance (new_reqs) > unit_req_imbalance (reqs)) > - regrename_do_replace (this_head, old_reg); > + gcc_assert (regrename_do_replace (this_head, old_reg)); > else > memcpy (reqs, new_reqs, sizeof (unit_req_table)); > I'm sorry; do you have a suggestion for a fix? I thought this was the change I was asked to make, and as I noted previously, I'm not set up to test (or even build) for this target. -Sandra the obviously confused :-( ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [patch] fix regrename pass to ensure renamings produce valid insns 2015-06-30 5:08 ` Sandra Loosemore @ 2015-06-30 5:26 ` Chung-Lin Tang 2015-06-30 6:11 ` Chung-Lin Tang 0 siblings, 1 reply; 25+ messages in thread From: Chung-Lin Tang @ 2015-06-30 5:26 UTC (permalink / raw) To: Sandra Loosemore, Kito Cheng Cc: Jeff Law, Eric Botcazou, gcc-patches, Marcus Shawcroft, Richard Earnshaw, Schmidt, Bernd - Code Sourcery On 2015/6/30 12:22 PM, Sandra Loosemore wrote: > On 06/29/2015 09:07 PM, Kito Cheng wrote: >> Hi all: >> >> This patch seem will broken when disable assert checking for c6x.... >> >> Index: gcc/config/c6x/c6x.c >> =================================================================== >> --- gcc/config/c6x/c6x.c (revision 225104) >> +++ gcc/config/c6x/c6x.c (working copy) >> @@ -3516,7 +3516,7 @@ try_rename_operands (rtx_insn *head, rtx >> best_reg = >> find_rename_reg (this_head, super_class, &unavailable, old_reg, true); >> >> - regrename_do_replace (this_head, best_reg); >> + gcc_assert (regrename_do_replace (this_head, best_reg)); >> >> count_unit_reqs (new_reqs, head, PREV_INSN (tail)); >> merge_unit_reqs (new_reqs); >> @@ -3529,7 +3529,7 @@ try_rename_operands (rtx_insn *head, rtx >> unit_req_imbalance (reqs), unit_req_imbalance (new_reqs)); >> } >> if (unit_req_imbalance (new_reqs) > unit_req_imbalance (reqs)) >> - regrename_do_replace (this_head, old_reg); >> + gcc_assert (regrename_do_replace (this_head, old_reg)); >> else >> memcpy (reqs, new_reqs, sizeof (unit_req_table)); >> > > I'm sorry; do you have a suggestion for a fix? I thought this was the change I was asked to make, and as I noted previously, I'm not set up to test (or even build) for this target. > > -Sandra the obviously confused :-( You probably have to separate out the regrename_do_replace() bool result into a variable, placing the whole call into the gcc_assert() might make it disappear when assertions are turned off. Chung-Lin ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [patch] fix regrename pass to ensure renamings produce valid insns 2015-06-30 5:26 ` Chung-Lin Tang @ 2015-06-30 6:11 ` Chung-Lin Tang 2015-06-30 9:08 ` Eric Botcazou 0 siblings, 1 reply; 25+ messages in thread From: Chung-Lin Tang @ 2015-06-30 6:11 UTC (permalink / raw) To: Sandra Loosemore, Kito Cheng Cc: Jeff Law, Eric Botcazou, gcc-patches, Marcus Shawcroft, Richard Earnshaw, Schmidt, Bernd - Code Sourcery On 2015/6/30 ä¸å 01:13, Chung-Lin Tang wrote: > On 2015/6/30 12:22 PM, Sandra Loosemore wrote: >> On 06/29/2015 09:07 PM, Kito Cheng wrote: >>> Hi all: >>> >>> This patch seem will broken when disable assert checking for c6x.... >>> >>> Index: gcc/config/c6x/c6x.c >>> =================================================================== >>> --- gcc/config/c6x/c6x.c (revision 225104) >>> +++ gcc/config/c6x/c6x.c (working copy) >>> @@ -3516,7 +3516,7 @@ try_rename_operands (rtx_insn *head, rtx >>> best_reg = >>> find_rename_reg (this_head, super_class, &unavailable, old_reg, true); >>> >>> - regrename_do_replace (this_head, best_reg); >>> + gcc_assert (regrename_do_replace (this_head, best_reg)); >>> >>> count_unit_reqs (new_reqs, head, PREV_INSN (tail)); >>> merge_unit_reqs (new_reqs); >>> @@ -3529,7 +3529,7 @@ try_rename_operands (rtx_insn *head, rtx >>> unit_req_imbalance (reqs), unit_req_imbalance (new_reqs)); >>> } >>> if (unit_req_imbalance (new_reqs) > unit_req_imbalance (reqs)) >>> - regrename_do_replace (this_head, old_reg); >>> + gcc_assert (regrename_do_replace (this_head, old_reg)); >>> else >>> memcpy (reqs, new_reqs, sizeof (unit_req_table)); >>> >> >> I'm sorry; do you have a suggestion for a fix? I thought this was the change I was asked to make, and as I noted previously, I'm not set up to test (or even build) for this target. >> >> -Sandra the obviously confused :-( > > You probably have to separate out the regrename_do_replace() bool result into a variable, > placing the whole call into the gcc_assert() might make it disappear when assertions are turned off. I notice the way gcc_assert() is defined in system.h now, the test won't disappear even when runtime checks are disabled, though you might still adjust it to avoid any programmer confusion. Chung-Lin ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [patch] fix regrename pass to ensure renamings produce valid insns 2015-06-30 6:11 ` Chung-Lin Tang @ 2015-06-30 9:08 ` Eric Botcazou 2015-06-30 11:03 ` Chung-Lin Tang 2015-06-30 19:11 ` Sandra Loosemore 0 siblings, 2 replies; 25+ messages in thread From: Eric Botcazou @ 2015-06-30 9:08 UTC (permalink / raw) To: Chung-Lin Tang Cc: gcc-patches, Sandra Loosemore, Kito Cheng, Jeff Law, Marcus Shawcroft, Richard Earnshaw, Schmidt, Bernd - Code Sourcery > I notice the way gcc_assert() is defined in system.h now, the test won't > disappear even when runtime checks are disabled, though you might still > adjust it to avoid any programmer confusion. It will disappear at run time, see the definition: /* Include EXPR, so that unused variable warnings do not occur. */ #define gcc_assert(EXPR) ((void)(0 && (EXPR))) so you really need to use a separate variable. -- Eric Botcazou ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [patch] fix regrename pass to ensure renamings produce valid insns 2015-06-30 9:08 ` Eric Botcazou @ 2015-06-30 11:03 ` Chung-Lin Tang 2015-06-30 19:11 ` Sandra Loosemore 1 sibling, 0 replies; 25+ messages in thread From: Chung-Lin Tang @ 2015-06-30 11:03 UTC (permalink / raw) To: Eric Botcazou Cc: gcc-patches, Sandra Loosemore, Kito Cheng, Jeff Law, Marcus Shawcroft, Richard Earnshaw, Schmidt, Bernd - Code Sourcery On 2015/6/30 05:06 PM, Eric Botcazou wrote: >> I notice the way gcc_assert() is defined in system.h now, the test won't >> disappear even when runtime checks are disabled, though you might still >> adjust it to avoid any programmer confusion. > > It will disappear at run time, see the definition: > > /* Include EXPR, so that unused variable warnings do not occur. */ > #define gcc_assert(EXPR) ((void)(0 && (EXPR))) > > so you really need to use a separate variable. > I was referring to this one: #if ENABLE_ASSERT_CHECKING ... #elif (GCC_VERSION >= 4005) #define gcc_assert(EXPR) \ ((void)(__builtin_expect (!(EXPR), 0) ? __builtin_unreachable (), 0 : 0)) #else ... But yeah, I guess older GCCs could be used to build a toolchain, so a separate variable should be used. Chung-Lin ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [patch] fix regrename pass to ensure renamings produce valid insns 2015-06-30 9:08 ` Eric Botcazou 2015-06-30 11:03 ` Chung-Lin Tang @ 2015-06-30 19:11 ` Sandra Loosemore 2015-06-30 21:31 ` Eric Botcazou 1 sibling, 1 reply; 25+ messages in thread From: Sandra Loosemore @ 2015-06-30 19:11 UTC (permalink / raw) To: Eric Botcazou Cc: Chung-Lin Tang, gcc-patches, Kito Cheng, Jeff Law, Marcus Shawcroft, Richard Earnshaw, Schmidt, Bernd - Code Sourcery [-- Attachment #1: Type: text/plain, Size: 992 bytes --] On 06/30/2015 03:06 AM, Eric Botcazou wrote: >> I notice the way gcc_assert() is defined in system.h now, the test won't >> disappear even when runtime checks are disabled, though you might still >> adjust it to avoid any programmer confusion. > > It will disappear at run time, see the definition: > > /* Include EXPR, so that unused variable warnings do not occur. */ > #define gcc_assert(EXPR) ((void)(0 && (EXPR))) > > so you really need to use a separate variable. Oh, yuck -- it never even occurred to me that gcc_assert could be disabled. I'll bet there are other bugs in GCC due to this very same problem of depending on its argument being executed for side-effect. (E.g. take a look at add_stmt_to_eh_lp_fn in tree-eh.c.) Seems like lousy design to me especially since proper usage doesn't seem to be documented anywhere. Anyway, I think the attached patch is what's required to fix the instance that's my fault. OK? Bernd, if this needs testing, can you help? -Sandra [-- Attachment #2: regrename-4.log --] [-- Type: text/x-log, Size: 175 bytes --] 2015-06-30 Sandra Loosemore <sandra@codesourcery.com> gcc/ * config/c6x/c6x.c (try_rename_operands): Do not depend on gcc_assert evaluating its argument for side-effect. [-- Attachment #3: regrename-4.patch --] [-- Type: text/x-patch, Size: 1209 bytes --] Index: gcc/config/c6x/c6x.c =================================================================== --- gcc/config/c6x/c6x.c (revision 225202) +++ gcc/config/c6x/c6x.c (working copy) @@ -3450,6 +3450,7 @@ try_rename_operands (rtx_insn *head, rtx int best_reg, old_reg; vec<du_head_p> involved_chains = vNULL; unit_req_table new_reqs; + bool ok; for (i = 0, tmp_mask = op_mask; tmp_mask; i++) { @@ -3516,7 +3517,8 @@ try_rename_operands (rtx_insn *head, rtx best_reg = find_rename_reg (this_head, super_class, &unavailable, old_reg, true); - gcc_assert (regrename_do_replace (this_head, best_reg)); + ok = regrename_do_replace (this_head, best_reg); + gcc_assert (ok); count_unit_reqs (new_reqs, head, PREV_INSN (tail)); merge_unit_reqs (new_reqs); @@ -3529,7 +3531,10 @@ try_rename_operands (rtx_insn *head, rtx unit_req_imbalance (reqs), unit_req_imbalance (new_reqs)); } if (unit_req_imbalance (new_reqs) > unit_req_imbalance (reqs)) - gcc_assert (regrename_do_replace (this_head, old_reg)); + { + ok = regrename_do_replace (this_head, old_reg); + gcc_assert (ok); + } else memcpy (reqs, new_reqs, sizeof (unit_req_table)); ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [patch] fix regrename pass to ensure renamings produce valid insns 2015-06-30 19:11 ` Sandra Loosemore @ 2015-06-30 21:31 ` Eric Botcazou 0 siblings, 0 replies; 25+ messages in thread From: Eric Botcazou @ 2015-06-30 21:31 UTC (permalink / raw) To: Sandra Loosemore Cc: gcc-patches, Chung-Lin Tang, Kito Cheng, Jeff Law, Marcus Shawcroft, Richard Earnshaw, Schmidt, Bernd - Code Sourcery > Oh, yuck -- it never even occurred to me that gcc_assert could be > disabled. I'll bet there are other bugs in GCC due to this very same > problem of depending on its argument being executed for side-effect. Very likely so... > (E.g. take a look at add_stmt_to_eh_lp_fn in tree-eh.c.) Seems like > lousy design to me especially since proper usage doesn't seem to be > documented anywhere. ... but not this one though. > Anyway, I think the attached patch is what's required to fix the > instance that's my fault. OK? Yes, it's obviously OK. -- Eric Botcazou ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [patch] fix regrename pass to ensure renamings produce valid insns 2015-06-29 0:18 ` Sandra Loosemore 2015-06-30 3:54 ` Kito Cheng @ 2015-07-01 17:03 ` Jeff Law 1 sibling, 0 replies; 25+ messages in thread From: Jeff Law @ 2015-07-01 17:03 UTC (permalink / raw) To: Sandra Loosemore Cc: Eric Botcazou, gcc-patches, Chung-Lin Tang, Marcus Shawcroft, Richard Earnshaw, Schmidt, Bernd - Code Sourcery On 06/28/2015 03:08 PM, Sandra Loosemore wrote: > > Re the testcase, this fixed 16 FAILs on existing tests in the gcc > testsuite with the forthcoming nios2 load/store multiple instruction > support, all assembler errors due to the bad instructions being > generated. There's nothing I can do on nios2 for a testcase until I get > those patches committed (I'm still trying to re-test and tidy them up > for submission), plus I think the failures are rather fragile -- > depending on the register allocator choosing an initial register > numbering that allows peephole optimizers to trigger, etc. But, I will > revisit this later and see what I can do. OK. As long as there's going to be some tests, that works for me. jeff ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [patch] fix regrename pass to ensure renamings produce valid insns 2015-06-17 17:36 [patch] fix regrename pass to ensure renamings produce valid insns Sandra Loosemore 2015-06-17 19:23 ` Richard Sandiford 2015-06-18 18:26 ` Eric Botcazou @ 2015-11-06 10:48 ` Bernd Schmidt 2015-11-06 19:51 ` Jeff Law 2 siblings, 1 reply; 25+ messages in thread From: Bernd Schmidt @ 2015-11-06 10:48 UTC (permalink / raw) To: Sandra Loosemore, GCC Patches; +Cc: Chung-Lin Tang [-- Attachment #1: Type: text/plain, Size: 1257 bytes --] On 06/17/2015 07:11 PM, Sandra Loosemore wrote: > Index: gcc/regrename.c > =================================================================== > --- gcc/regrename.c (revision 224532) > +++ gcc/regrename.c (working copy) > @@ -942,19 +942,22 @@ 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; With a patch I'm working on that uses the renamer more often, I found that this is causing compare-debug failures. Validating changes to debug_insns (the INSN_VAR_LOCATION_LOC in particular) can apparently fail. The following fix was bootstrapped and tested with -frename-registers enabled at -O1 on x86_64-linux. Ok? Bernd [-- Attachment #2: rr-dinsn.diff --] [-- Type: text/x-patch, Size: 1313 bytes --] * regrename.c (regrename_do_replace): Do not validate changes to debug insns. Index: gcc/regrename.c =================================================================== --- gcc/regrename.c (revision 229049) +++ gcc/regrename.c (working copy) @@ -946,10 +951,7 @@ regrename_do_replace (struct du_head *he struct reg_attrs *attr = REG_ATTRS (*chain->loc); int reg_ptr = REG_POINTER (*chain->loc); - if (DEBUG_INSN_P (chain->insn) && REGNO (*chain->loc) != base_regno) - validate_change (chain->insn, &(INSN_VAR_LOCATION_LOC (chain->insn)), - gen_rtx_UNKNOWN_VAR_LOC (), true); - else + if (!DEBUG_INSN_P (chain->insn)) { validate_change (chain->insn, chain->loc, gen_raw_REG (GET_MODE (*chain->loc), reg), true); @@ -963,6 +965,16 @@ regrename_do_replace (struct du_head *he if (!apply_change_group ()) return false; + for (chain = head->first; chain; chain = chain->next_use) + if (DEBUG_INSN_P (chain->insn)) + { + if (REGNO (*chain->loc) != base_regno) + INSN_VAR_LOCATION_LOC (chain->insn) = gen_rtx_UNKNOWN_VAR_LOC (); + else + *chain->loc = gen_raw_REG (GET_MODE (*chain->loc), reg); + df_insn_rescan (chain->insn); + } + mode = GET_MODE (*head->first->loc); head->regno = reg; head->nregs = hard_regno_nregs[reg][mode]; ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [patch] fix regrename pass to ensure renamings produce valid insns 2015-11-06 10:48 ` Bernd Schmidt @ 2015-11-06 19:51 ` Jeff Law 2015-11-13 15:13 ` Bernd Schmidt 0 siblings, 1 reply; 25+ messages in thread From: Jeff Law @ 2015-11-06 19:51 UTC (permalink / raw) To: Bernd Schmidt, Sandra Loosemore, GCC Patches; +Cc: Chung-Lin Tang On 11/06/2015 03:48 AM, Bernd Schmidt wrote: > On 06/17/2015 07:11 PM, Sandra Loosemore wrote: > >> Index: gcc/regrename.c >> =================================================================== >> --- gcc/regrename.c (revision 224532) >> +++ gcc/regrename.c (working copy) >> @@ -942,19 +942,22 @@ 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; > > With a patch I'm working on that uses the renamer more often, I found > that this is causing compare-debug failures. Validating changes to > debug_insns (the INSN_VAR_LOCATION_LOC in particular) can apparently fail. > > The following fix was bootstrapped and tested with -frename-registers > enabled at -O1 on x86_64-linux. Ok? Essentially we're keeping the validate_change that Sandra & Chung added on the real insns to address the problem on the nios2 port. The patch just drops the DEBUG_INSN changes from the change group. For the DEBUG_INSNS in the chain, we update those independently after we apply the change group. I think the change is fine for the trunk, though I'm still curious about how the code as-is resulted in a comparison failure. jeff ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [patch] fix regrename pass to ensure renamings produce valid insns 2015-11-06 19:51 ` Jeff Law @ 2015-11-13 15:13 ` Bernd Schmidt 0 siblings, 0 replies; 25+ messages in thread From: Bernd Schmidt @ 2015-11-13 15:13 UTC (permalink / raw) To: Jeff Law, Bernd Schmidt, Sandra Loosemore, GCC Patches; +Cc: Chung-Lin Tang On 11/06/2015 08:51 PM, Jeff Law wrote: > I think the change is fine for the trunk, though I'm still curious about > how the code as-is resulted in a comparison failure. I've been retesting and I think this was a case of something else triggering an random failure - the patch made it go away on the testcase I looked at, but random compare-debug failures persisted. I think I have those fixed now. I'll leave this patch out for now. Bernd ^ permalink raw reply [flat|nested] 25+ messages in thread
end of thread, other threads:[~2015-11-13 15:13 UTC | newest] Thread overview: 25+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2015-06-17 17:36 [patch] fix regrename pass to ensure renamings produce valid insns Sandra Loosemore 2015-06-17 19:23 ` Richard Sandiford 2015-06-18 18:26 ` Eric Botcazou 2015-06-24 3:31 ` Sandra Loosemore 2015-06-24 7:59 ` Ramana Radhakrishnan 2015-06-24 14:57 ` Sandra Loosemore 2015-06-24 16:50 ` Eric Botcazou 2015-06-24 16:59 ` Eric Botcazou 2015-06-25 3:53 ` Jeff Law 2015-06-25 12:30 ` Bernd Schmidt 2015-06-25 13:54 ` Eric Botcazou 2015-06-25 13:59 ` Bernd Schmidt 2015-06-29 0:18 ` Sandra Loosemore 2015-06-30 3:54 ` Kito Cheng 2015-06-30 5:08 ` Sandra Loosemore 2015-06-30 5:26 ` Chung-Lin Tang 2015-06-30 6:11 ` Chung-Lin Tang 2015-06-30 9:08 ` Eric Botcazou 2015-06-30 11:03 ` Chung-Lin Tang 2015-06-30 19:11 ` Sandra Loosemore 2015-06-30 21:31 ` Eric Botcazou 2015-07-01 17:03 ` Jeff Law 2015-11-06 10:48 ` Bernd Schmidt 2015-11-06 19:51 ` Jeff Law 2015-11-13 15:13 ` Bernd Schmidt
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).