Re: [patch] fix regrename pass to ensure renamings produce valid insns
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
Re: [patch] fix regrename pass to ensure renamings produce valid insns
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
* 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];
Re: [patch] fix regrename pass to ensure renamings produce valid insns
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
Re: [patch] fix regrename pass to ensure renamings produce valid insns
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
Re: [patch] fix regrename pass to ensure renamings produce valid insns
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
Re: [patch] fix regrename pass to ensure renamings produce valid insns
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
2015-06-30 Sandra Loosemore san...@codesourcery.com
gcc/
* config/c6x/c6x.c (try_rename_operands): Do not depend on
gcc_assert evaluating its argument for side-effect.
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;
vecdu_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));
Re: [patch] fix regrename pass to ensure renamings produce valid insns
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
Re: [patch] fix regrename pass to ensure renamings produce valid insns
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
Re: [patch] fix regrename pass to ensure renamings produce valid insns
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 san...@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 Tangclt...@codesourcery.com Sandra Loosemoresan...@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
Re: [patch] fix regrename pass to ensure renamings produce valid insns
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 :-(
Re: [patch] fix regrename pass to ensure renamings produce valid insns
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
Re: [patch] fix regrename pass to ensure renamings produce valid insns
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
Re: [patch] fix regrename pass to ensure renamings produce valid insns
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 Tangclt...@codesourcery.com
Sandra Loosemoresan...@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
2015-06-28 Chung-Lin Tang clt...@codesourcery.com
Sandra Loosemore san...@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.
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
Re: [patch] fix regrename pass to ensure renamings produce valid insns
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
Re: [patch] fix regrename pass to ensure renamings produce valid insns
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
Re: [patch] fix regrename pass to ensure renamings produce valid insns
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
Re: [patch] fix regrename pass to ensure renamings produce valid insns
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
Re: [patch] fix regrename pass to ensure renamings produce valid insns
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
Re: [patch] fix regrename pass to ensure renamings produce valid insns
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
Re: [patch] fix regrename pass to ensure renamings produce valid insns
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 Tangclt...@codesourcery.com Sandra Loosemoresan...@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
Re: [patch] fix regrename pass to ensure renamings produce valid insns
Yes, the patch is OK, modulo... But you also need the approval of an ARM maintainer. -- Eric Botcazou
Re: [patch] fix regrename pass to ensure renamings produce valid insns
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
2015-06-23 Chung-Lin Tang clt...@codesourcery.com
Sandra Loosemore san...@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.
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
+{
+
Re: [patch] fix regrename pass to ensure renamings produce valid insns
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
Re: [patch] fix regrename pass to ensure renamings produce valid insns
Sandra Loosemore san...@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
[patch] fix regrename pass to ensure renamings produce valid insns
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
2015-06-17 Chung-Lin Tang clt...@codesourcery.com
Sandra Loosemore san...@codesourcery.com
gcc/
* regrename.c (regrename_do_replace): Re-validate the modified
insns.
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];
