On 03/04/2019 07:41 PM, Peter Bergner wrote:
On 3/4/19 4:24 PM, Peter Bergner wrote:
On 3/4/19 4:16 PM, Peter Bergner wrote:
Index: gcc/config/rs6000/rs6000.c
===================================================================
--- gcc/config/rs6000/rs6000.c (revision 269028)
+++ gcc/config/rs6000/rs6000.c (working copy)
@@ -9887,7 +9887,7 @@ valid_sf_si_move (rtx dest, rtx src, mac
static bool
rs6000_emit_move_si_sf_subreg (rtx dest, rtx source, machine_mode mode)
{
- if (TARGET_DIRECT_MOVE_64BIT && !lra_in_progress && !reload_completed
+ if (TARGET_DIRECT_MOVE_64BIT && !reload_completed
&& (!SUBREG_P (dest) || !sf_subreg_operand (dest, mode))
&& SUBREG_P (source) && sf_subreg_operand (source, mode))
{
@@ -9902,7 +9902,9 @@ rs6000_emit_move_si_sf_subreg (rtx dest,
if (mode == SFmode && inner_mode == SImode)
{
- emit_insn (gen_movsf_from_si (dest, inner_source));
+ rtx_insn *insn = emit_insn (gen_movsf_from_si (dest, inner_source));
+ if (lra_in_progress)
+ remove_scratches_1 (insn);
return true;
}
}
But maybe the call to remove_scratches_1() should move to lra_emit_move(),
which is how we get to this code in the first place? Who knows what other
generic move patterns might need scratches too?
Like this. This bootstraps and regtests with no regressions. Do you prefer
this instead? If so, we'll need Vlad or Jeff or ... to approve the LRA
changes.
Vlad and Jeff,
The original problem and patch is described here:
https://gcc.gnu.org/ml/gcc-patches/2019-03/msg00061.html
Short answer is, after enabling a rs6000 move pattern we need for spilling,
we ICE when spilling, because the move pattern uses a scratch register
and scratch registers are replaced early on during LRA initialization.
The patch below just extracts out the code that fixes up one insn and
makes it a function itself. I then changed lra_emit_move() to then call
that function after generating the move insn so as to replace the scratch
register the move pattern generated. Thoughts on this patch compared to
the rs6000 only patch linked above?
I recommend don't use scratches at all because IRA ignores them and it
might result in worse allocation. Unfortunately, removing scratches for
all targets requires a lot of work. This patch deals with scratches
during all LRA work which is good. So I like the patch and LRA part of
it is ok to commit.
Peter, thank you for adding the new functionality to LRA.
gcc/
PR rtl-optimization/88845
* config/rs6000/rs6000.c (rs6000_emit_move_si_sf_subreg): Enable during
LRA.
* lra.c (remove_scratches_1): New function.
(remove_scratches): Use it.
(lra_emit_move): Likewise.
gcc/testsuite/
PR rtl-optimization/88845
* gcc.target/powerpc/pr88845.c: New test.
Index: gcc/lra.c
===================================================================
--- gcc/lra.c (revision 269263)
+++ gcc/lra.c (working copy)
@@ -159,6 +159,7 @@ static void invalidate_insn_recog_data (
static int get_insn_freq (rtx_insn *);
static void invalidate_insn_data_regno_info (lra_insn_recog_data_t,
rtx_insn *, int);
+static void remove_scratches_1 (rtx_insn *);
/* Expand all regno related info needed for LRA. */
static void
@@ -494,7 +495,11 @@ lra_emit_move (rtx x, rtx y)
if (rtx_equal_p (x, y))
return;
old = max_reg_num ();
- emit_move_insn (x, y);
+ rtx_insn *insn = emit_move_insn (x, y);
+ /* The move pattern may require scratch registers, so convert them
+ into real registers now. */
+ if (insn != NULL_RTX)
+ remove_scratches_1 (insn);
if (REG_P (x))
lra_reg_info[ORIGINAL_REGNO (x)].last_reload = ++lra_curr_reload_num;
/* Function emit_move can create pseudos -- so expand the pseudo
@@ -2077,47 +2082,53 @@ lra_register_new_scratch_op (rtx_insn *i
add_reg_note (insn, REG_UNUSED, op);
}
-/* Change scratches onto pseudos and save their location. */
+/* Change INSN's scratches into pseudos and save their location. */
static void
-remove_scratches (void)
+remove_scratches_1 (rtx_insn *insn)
{
int i;
bool insn_changed_p;
- basic_block bb;
- rtx_insn *insn;
rtx reg;
lra_insn_recog_data_t id;
struct lra_static_insn_data *static_id;
+ id = lra_get_insn_recog_data (insn);
+ static_id = id->insn_static_data;
+ insn_changed_p = false;
+ for (i = 0; i < static_id->n_operands; i++)
+ if (GET_CODE (*id->operand_loc[i]) == SCRATCH
+ && GET_MODE (*id->operand_loc[i]) != VOIDmode)
+ {
+ insn_changed_p = true;
+ *id->operand_loc[i] = reg
+ = lra_create_new_reg (static_id->operand[i].mode,
+ *id->operand_loc[i], ALL_REGS, NULL);
+ lra_register_new_scratch_op (insn, i, id->icode);
+ if (lra_dump_file != NULL)
+ fprintf (lra_dump_file,
+ "Removing SCRATCH in insn #%u (nop %d)\n",
+ INSN_UID (insn), i);
+ }
+ if (insn_changed_p)
+ /* Because we might use DF right after caller-saves sub-pass
+ we need to keep DF info up to date. */
+ df_insn_rescan (insn);
+}
+
+/* Change scratches into pseudos and save their location. */
+static void
+remove_scratches (void)
+{
+ basic_block bb;
+ rtx_insn *insn;
+
scratches.create (get_max_uid ());
bitmap_initialize (&scratch_bitmap, ®_obstack);
bitmap_initialize (&scratch_operand_bitmap, ®_obstack);
FOR_EACH_BB_FN (bb, cfun)
FOR_BB_INSNS (bb, insn)
if (INSN_P (insn))
- {
- id = lra_get_insn_recog_data (insn);
- static_id = id->insn_static_data;
- insn_changed_p = false;
- for (i = 0; i < static_id->n_operands; i++)
- if (GET_CODE (*id->operand_loc[i]) == SCRATCH
- && GET_MODE (*id->operand_loc[i]) != VOIDmode)
- {
- insn_changed_p = true;
- *id->operand_loc[i] = reg
- = lra_create_new_reg (static_id->operand[i].mode,
- *id->operand_loc[i], ALL_REGS, NULL);
- lra_register_new_scratch_op (insn, i, id->icode);
- if (lra_dump_file != NULL)
- fprintf (lra_dump_file,
- "Removing SCRATCH in insn #%u (nop %d)\n",
- INSN_UID (insn), i);
- }
- if (insn_changed_p)
- /* Because we might use DF right after caller-saves sub-pass
- we need to keep DF info up to date. */
- df_insn_rescan (insn);
- }
+ remove_scratches_1 (insn);
}
