This is the mail archive of the
gcc-patches@gcc.gnu.org
mailing list for the GCC project.
[PATCH] Follow-up regcprop fixes
On Wed, Mar 27, 2019 at 04:32:15PM +0100, Jakub Jelinek wrote:
> On Wed, Mar 27, 2019 at 09:24:46AM -0600, Jeff Law wrote:
> > > 2) another thing I've noticed today, we queue up the debug insn changes,
> > > but if we iterate the second time for any bb, we overwrite and thus lose any
> > > of the debug insn queued changes from the first iteration, so those changes
> > > aren't applied (wrong-debug?)
> > This is actually what worries me, both with the current bits and with
> > any bits that change to a worklist of blocks to reprocess. As is I
> > think we preserve the debug stuff across the iterations which should
> > keep debug info correct. Managing that in a world where we queue up the
> > iteration step is going to be tougher.
>
> The patch I've posted would do df_analyze, all bbs once, then df_analyze,
> process queued debug changes, and if anything needs to be repeated (just
> once), redo the bbs from worklist and repeat the above.
> That seems to me the easiest way to get rid of the per-bb df_analyze calls
> as well as fixing the debug stuff. Because otherwise, we'd need to somehow
> merge the queued debug insns from the first and second pass and figure out
> how to deal with that.
Here is everything in patch on top of current trunk which contains your
regcprop.c changes.
In addition to the previously mentioned changes, as you've requested it
clears the visited sbitmap between the two passes, fixes clearing of the
all_vd[bb->index].e[regno].debug_insn_changes pointers after
cprop_hardreg_debug processes them and fixes up the n_debug_insn_changes
updates, so that the invariant that it always is the sum of length of the
debug_insn_changes chains for all hard regs in the bb is true (before that
n_debug_insn_changes could be higher, and in the final debug insn processing
case wasn't actually decremented at all, so the == 0 check was useless
there).
Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk if it
succeeds on some other targets too?
2019-03-27 Jakub Jelinek <jakub@redhat.com>
* regcprop.c (copyprop_hardreg_forward_1): Remove redundant INSN_P
test.
(cprop_hardreg_bb, cprop_hardreg_debug): New functions.
(pass_cprop_hardreg::execute): Use those. Don't repeat bb processing
immediately after first one with df_analyze in between, but rather
process all bbs, queueing ones that need second pass in a worklist,
df_analyze, process queued debug insn changes and if second pass is
needed, process bbs from worklist, df_analyze, process queued debug
insns again.
--- gcc/regcprop.c.jj 2019-03-27 18:13:45.296783680 +0100
+++ gcc/regcprop.c 2019-03-27 21:45:15.382232151 +0100
@@ -801,7 +801,6 @@ copyprop_hardreg_forward_1 (basic_block
/* Detect obviously dead sets (via REG_UNUSED notes) and remove them. */
if (set
&& !RTX_FRAME_RELATED_P (insn)
- && INSN_P (insn)
&& !may_trap_p (set)
&& find_reg_note (insn, REG_UNUSED, SET_DEST (set))
&& !side_effects_p (SET_SRC (set))
@@ -1282,6 +1281,76 @@ public:
}; // class pass_cprop_hardreg
+static bool
+cprop_hardreg_bb (basic_block bb, struct value_data *all_vd, sbitmap visited)
+{
+ bitmap_set_bit (visited, bb->index);
+
+ /* If a block has a single predecessor, that we've already
+ processed, begin with the value data that was live at
+ the end of the predecessor block. */
+ /* ??? Ought to use more intelligent queuing of blocks. */
+ if (single_pred_p (bb)
+ && bitmap_bit_p (visited, single_pred (bb)->index)
+ && ! (single_pred_edge (bb)->flags & (EDGE_ABNORMAL_CALL | EDGE_EH)))
+ {
+ all_vd[bb->index] = all_vd[single_pred (bb)->index];
+ if (all_vd[bb->index].n_debug_insn_changes)
+ {
+ unsigned int regno;
+
+ for (regno = 0; regno < FIRST_PSEUDO_REGISTER; regno++)
+ {
+ if (all_vd[bb->index].e[regno].debug_insn_changes)
+ {
+ struct queued_debug_insn_change *cur;
+ for (cur = all_vd[bb->index].e[regno].debug_insn_changes;
+ cur; cur = cur->next)
+ --all_vd[bb->index].n_debug_insn_changes;
+ all_vd[bb->index].e[regno].debug_insn_changes = NULL;
+ if (all_vd[bb->index].n_debug_insn_changes == 0)
+ break;
+ }
+ }
+ }
+ }
+ else
+ init_value_data (all_vd + bb->index);
+
+ return copyprop_hardreg_forward_1 (bb, all_vd + bb->index);
+}
+
+static void
+cprop_hardreg_debug (function *fun, struct value_data *all_vd)
+{
+ basic_block bb;
+
+ FOR_EACH_BB_FN (bb, fun)
+ if (all_vd[bb->index].n_debug_insn_changes)
+ {
+ unsigned int regno;
+ bitmap live;
+
+ live = df_get_live_out (bb);
+ for (regno = 0; regno < FIRST_PSEUDO_REGISTER; regno++)
+ if (all_vd[bb->index].e[regno].debug_insn_changes)
+ {
+ if (REGNO_REG_SET_P (live, regno))
+ apply_debug_insn_changes (all_vd + bb->index, regno);
+
+ struct queued_debug_insn_change *cur;
+ for (cur = all_vd[bb->index].e[regno].debug_insn_changes;
+ cur; cur = cur->next)
+ --all_vd[bb->index].n_debug_insn_changes;
+ all_vd[bb->index].e[regno].debug_insn_changes = NULL;
+ if (all_vd[bb->index].n_debug_insn_changes == 0)
+ break;
+ }
+ }
+
+ queued_debug_insn_change_pool.release ();
+}
+
unsigned int
pass_cprop_hardreg::execute (function *fun)
{
@@ -1293,6 +1362,9 @@ pass_cprop_hardreg::execute (function *f
auto_sbitmap visited (last_basic_block_for_fn (fun));
bitmap_clear (visited);
+ auto_vec<int> worklist;
+ bool any_debug_changes = false;
+
/* We need accurate notes. Earlier passes such as if-conversion may
leave notes in an inconsistent state. */
df_note_add_problem ();
@@ -1310,69 +1382,39 @@ pass_cprop_hardreg::execute (function *f
FOR_EACH_BB_FN (bb, fun)
{
- bitmap_set_bit (visited, bb->index);
-
- for (int pass = 0; pass < 2; pass++)
- {
- /* If a block has a single predecessor, that we've already
- processed, begin with the value data that was live at
- the end of the predecessor block. */
- /* ??? Ought to use more intelligent queuing of blocks. */
- if (single_pred_p (bb)
- && bitmap_bit_p (visited, single_pred (bb)->index)
- && ! (single_pred_edge (bb)->flags & (EDGE_ABNORMAL_CALL | EDGE_EH)))
- {
- all_vd[bb->index] = all_vd[single_pred (bb)->index];
- if (all_vd[bb->index].n_debug_insn_changes)
- {
- unsigned int regno;
-
- for (regno = 0; regno < FIRST_PSEUDO_REGISTER; regno++)
- {
- if (all_vd[bb->index].e[regno].debug_insn_changes)
- {
- all_vd[bb->index].e[regno].debug_insn_changes = NULL;
- if (--all_vd[bb->index].n_debug_insn_changes == 0)
- break;
- }
- }
- }
- }
- else
- init_value_data (all_vd + bb->index);
-
- /* If we were unable to propagate, then break the loop. */
- if (!copyprop_hardreg_forward_1 (bb, all_vd + bb->index))
- break;
- df_analyze ();
- }
+ if (cprop_hardreg_bb (bb, all_vd, visited))
+ worklist.safe_push (bb->index);
+ if (all_vd[bb->index].n_debug_insn_changes)
+ any_debug_changes = true;
}
/* We must call df_analyze here unconditionally to ensure that the
REG_UNUSED and REG_DEAD notes are consistent with and without -g. */
df_analyze ();
- if (MAY_HAVE_DEBUG_BIND_INSNS)
- {
- FOR_EACH_BB_FN (bb, fun)
- if (bitmap_bit_p (visited, bb->index)
- && all_vd[bb->index].n_debug_insn_changes)
- {
- unsigned int regno;
- bitmap live;
+ if (MAY_HAVE_DEBUG_BIND_INSNS && any_debug_changes)
+ cprop_hardreg_debug (fun, all_vd);
- live = df_get_live_out (bb);
- for (regno = 0; regno < FIRST_PSEUDO_REGISTER; regno++)
- if (all_vd[bb->index].e[regno].debug_insn_changes)
- {
- if (REGNO_REG_SET_P (live, regno))
- apply_debug_insn_changes (all_vd + bb->index, regno);
- if (all_vd[bb->index].n_debug_insn_changes == 0)
- break;
- }
- }
+ /* Second pass if we've changed anything, only for the bbs where we have
+ changed anything though. */
+ if (!worklist.is_empty ())
+ {
+ unsigned int i;
+ int index;
- queued_debug_insn_change_pool.release ();
+ any_debug_changes = false;
+ bitmap_clear (visited);
+ FOR_EACH_VEC_ELT (worklist, i, index)
+ {
+ bb = BASIC_BLOCK_FOR_FN (fun, index);
+ cprop_hardreg_bb (bb, all_vd, visited);
+ if (all_vd[bb->index].n_debug_insn_changes)
+ any_debug_changes = true;
+ }
+
+ df_analyze ();
+ if (MAY_HAVE_DEBUG_BIND_INSNS && any_debug_changes)
+ cprop_hardreg_debug (fun, all_vd);
}
free (all_vd);
Jakub