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<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.
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 (*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));
