On 5/8/19 2:30 AM, Richard Biener wrote:
On Tue, May 7, 2019 at 11:55 PM Jeff Law <l...@redhat.com> wrote:
On 5/7/19 3:45 AM, Richard Biener wrote:
On Tue, May 7, 2019 at 11:13 AM Aldy Hernandez <al...@redhat.com> wrote:
Hi.
We seem to have numerous copies of the same EH propagation cleanups
scattered throughout the compiler. The attached patch moves all the
logic into one class that allows for easy marking of statements and
automatic cleanup once it goes out of scope.
Tested on x86-64 Linux.
OK for trunk? (*)
Ugh :/
First of all I don't like the fact that the actual cleanup is done
upon constructor execution. Please make it explicit
and in the constructor assert that nothing is to be done.
I'm of a mixed mind here. I have railed against implicit code being run
behind my back for decades.
However as I've had to debug locking issues and the like in other C++
codebases I've become more and more of a fan of RAII and its basic
concepts. This has made me more open to code running behind my back
like this implicitly when the object gets destroyed.
There's something to be said for embedding this little class into other
objects like Aldy has done and just letting things clean up
automatically as the object goes out of scope. No more missing calls to
run the cleanup bits, it "just works".
But I won't object if you want it to be more explicit. I've been there
and understand why one might want the cleanup step to be explicit.
Then I'm not sure this is a 1:1 transform since for example
@@ -1061,8 +1173,6 @@
substitute_and_fold_dom_walker::before_dom_children (basic_block bb)
}
gimple *old_stmt = stmt;
- bool was_noreturn = (is_gimple_call (stmt)
- && gimple_call_noreturn_p (stmt));
/* Replace real uses in the statement. */
did_replace |= substitute_and_fold_engine->replace_uses_in (stmt);
@@ -1110,25 +1220,7 @@
substitute_and_fold_dom_walker::before_dom_children (basic_block bb)
/* Now cleanup. */
if (did_replace)
{
...
+ fixups.record_change (old_stmt, stmt);
here we no longer can reliably determine whether old_stmt was noreturn since
we substitute into stmt itself. It's no longer a correctness issue if
we do _not_
fixup_noreturn since we now have GF_CALL_CTRL_ALTERING, it's merely
an optimization issue. So there may be no testcase for this (previously such
cases ICEd).
But AFAICT we don't care in the case Aldy is changing. If we really
want to know if the old statement was a noreturn we can test prior to
queing it.
The code isn't doing what it did before after the change. That's a bug.
If this is indeed a problem in the cases that I changed, it's easily
fixed by marking the statement ahead of time and keeping track of the
noreturn bit (as I have done in the attached patch).
To be more explicit here - adding this kind of new and fancy C++ stuff
just for the sake of it, thereby adding yet _another_ way of handling these
things instead of forcing it a new way (remove the other APIs this
encapsulates) is very bad(TM) IMHO, both for maintainance and for
new people coming around trying to understand GCC.
I'm not adding them for the sake of it. I'm adding them because we have
5 copies of the virtually the same code, and if I add any (additional)
propagation smarts to the compiler, I'm going to have to add a 6th copy.
My patch abstracts away 4 out of the 5 versions, and I am volunteering
to fix the last one (forwprop) as an incremental patch.
I don't agree that this is worse. On the contrary, I am transforming
multiple copies of this:
- bool was_noreturn = (is_gimple_call (stmt)
- && gimple_call_noreturn_p (stmt));
...
...
- /* If we cleaned up EH information from the statement,
- remove EH edges. */
- if (maybe_clean_or_replace_eh_stmt (old_stmt, stmt))
- bitmap_set_bit (need_eh_cleanup, bb->index);
-
- /* If we turned a not noreturn call into a noreturn one
- schedule it for fixup. */
- if (!was_noreturn
- && is_gimple_call (stmt)
- && gimple_call_noreturn_p (stmt))
- stmts_to_fixup.safe_push (stmt);
-
- if (gimple_assign_single_p (stmt))
- {
- tree rhs = gimple_assign_rhs1 (stmt);
- if (TREE_CODE (rhs) == ADDR_EXPR)
- recompute_tree_invariant_for_addr_expr (rhs);
- }
- }
by this:
+ fixups.mark_stmt (old_stmt);
...
...
+ fixups.record_change (stmt);
And we no longer have to clean things up manually at scope end:
- if (!bitmap_empty_p (need_eh_cleanup))
- gimple_purge_all_dead_eh_edges (need_eh_cleanup);
-
- /* Fixup stmts that became noreturn calls. This may require splitting
- blocks and thus isn't possible during the dominator walk. Do this
- in reverse order so we don't inadvertedly remove a stmt we want to
- fixup by visiting a dominating now noreturn call first. */
- while (!stmts_to_fixup.is_empty ())
- {
- gimple *stmt = stmts_to_fixup.pop ();
- fixup_noreturn_call (stmt);
- }
Not to say that all the places that are refactored with this patch
look sligtly different and thus the pattern doesnt' exactly match
(leading to issues like the one I pointed out above).
If there are any discrepancies in measurable behavior between what we
had and what I am proposing, they are indeed bugs, and I will gladly
address them.
Aldy
So - please no.
Richard.
I'm also not sure I like to put all these (unrelated) things into a
single class,
it really also hides the details of what is performed immediately and what
delayed and what kind of changes - this makes understanding of pass
transforms hard.
On the other hand this class defines a contract for what it can and will
do for us and allows us to bring consistency in that handling. We
declare manual management of this stuff verboten. Ideally we'd declare
the class final and avoid derivation, but I doubt we can do that
immediately.
Jeff
* tree-ssa-propagate.h (class propagate_cleanups): New.
* tree-ssa-propagate.c (class substitute_and_fold_dom_walker):
Remove references to stmts_to_fixup and need_eh_cleanup.
(propagate_cleanups::~propagate_cleanups): New.
(propagate_cleanups::mark_stmt): New.
(propagate_cleanups::record_change): New.
(propagate_cleanups::record_eh_change): New.
(propagate_mark_stmt_for_cleanup): New.
(propagate_cleanup): New.
(substitute_and_fold_dom_walker::before_dom_children): Adjust for
propagate_cleanups class.
(substitute_and_fold_engine::substitute_and_fold): Remove manual
EH cleanups.
* gimple-ssa-evrp.c (class evrp_dom_walker): Adjust for
propagate_cleanups class.
(evrp_dom_walker::before_dom_children): Same.
(evrp_dom_walker::cleanup): Same.
* tree-ssa-dom.c: Remove need_eh_cleanup and need_noreturn_fixup
globals.
(class domfixups): New.
(class dom_opt_dom_walker): Adjust for propagate_cleanups class.
(pass_dominator::execute): Same.
(dom_opt_dom_walker::optimize_stmt): Same.
* tree-ssa-forwprop.c (tidy_after_forward_propagate_addr): Remove.
(forward_propagate_addr_expr_1): Call
propagate_mark_stmt_for_cleanup instead of
tidy_after_forward_propagate_addr.
(pass_forwprop::execute): Adjust for propagate_cleanups class.
* tree-ssa-sccvn.c (class eliminate_dom_walker): Same.
(class rpo_elim): Same.
(eliminate_dom_walker::eliminate_dom_walker): Same.
(eliminate_dom_walker::~eliminate_dom_walker): Same.
(eliminate_dom_walker::eliminate_stmt): Same.
(eliminate_dom_walker::eliminate_cleanup): Same.
(eliminate_with_rpo_vn): Same.
diff --git a/gcc/gimple-ssa-evrp.c b/gcc/gimple-ssa-evrp.c
index 96da79bf028..9c6d99377cf 100644
--- a/gcc/gimple-ssa-evrp.c
+++ b/gcc/gimple-ssa-evrp.c
@@ -73,11 +73,9 @@ public:
evrp_range_analyzer (true),
evrp_folder (evrp_range_analyzer.get_vr_values ())
{
- need_eh_cleanup = BITMAP_ALLOC (NULL);
}
~evrp_dom_walker ()
{
- BITMAP_FREE (need_eh_cleanup);
}
virtual edge before_dom_children (basic_block);
virtual void after_dom_children (basic_block);
@@ -85,9 +83,8 @@ public:
private:
DISABLE_COPY_AND_ASSIGN (evrp_dom_walker);
- bitmap need_eh_cleanup;
- auto_vec<gimple *> stmts_to_fixup;
auto_vec<gimple *> stmts_to_remove;
+ propagate_cleanups fixups;
class evrp_range_analyzer evrp_range_analyzer;
class evrp_folder evrp_folder;
@@ -128,8 +125,7 @@ evrp_dom_walker::before_dom_children (basic_block bb)
gimple *stmt = gsi_stmt (gsi);
tree output = NULL_TREE;
gimple *old_stmt = stmt;
- bool was_noreturn = (is_gimple_call (stmt)
- && gimple_call_noreturn_p (stmt));
+ fixups.mark_stmt (old_stmt);
if (dump_file && (dump_flags & TDF_DETAILS))
{
@@ -190,26 +186,7 @@ evrp_dom_walker::before_dom_children (basic_block bb)
}
if (did_replace)
- {
- /* If we cleaned up EH information from the statement,
- remove EH edges. */
- if (maybe_clean_or_replace_eh_stmt (old_stmt, stmt))
- bitmap_set_bit (need_eh_cleanup, bb->index);
-
- /* If we turned a not noreturn call into a noreturn one
- schedule it for fixup. */
- if (!was_noreturn
- && is_gimple_call (stmt)
- && gimple_call_noreturn_p (stmt))
- stmts_to_fixup.safe_push (stmt);
-
- if (gimple_assign_single_p (stmt))
- {
- tree rhs = gimple_assign_rhs1 (stmt);
- if (TREE_CODE (rhs) == ADDR_EXPR)
- recompute_tree_invariant_for_addr_expr (rhs);
- }
- }
+ fixups.record_change (stmt);
}
/* Visit BB successor PHI nodes and replace PHI args. */
@@ -275,19 +252,6 @@ evrp_dom_walker::cleanup (void)
}
}
- if (!bitmap_empty_p (need_eh_cleanup))
- gimple_purge_all_dead_eh_edges (need_eh_cleanup);
-
- /* Fixup stmts that became noreturn calls. This may require splitting
- blocks and thus isn't possible during the dominator walk. Do this
- in reverse order so we don't inadvertedly remove a stmt we want to
- fixup by visiting a dominating now noreturn call first. */
- while (!stmts_to_fixup.is_empty ())
- {
- gimple *stmt = stmts_to_fixup.pop ();
- fixup_noreturn_call (stmt);
- }
-
evrp_folder.vr_values->cleanup_edges_and_switches ();
}
diff --git a/gcc/tree-ssa-dom.c b/gcc/tree-ssa-dom.c
index b0d56fcf3e3..93e87641e8d 100644
--- a/gcc/tree-ssa-dom.c
+++ b/gcc/tree-ssa-dom.c
@@ -89,11 +89,6 @@ class edge_info
/* Track whether or not we have changed the control flow graph. */
static bool cfg_altered;
-/* Bitmap of blocks that have had EH statements cleaned. We should
- remove their dead edges eventually. */
-static bitmap need_eh_cleanup;
-static vec<gimple *> need_noreturn_fixup;
-
/* Statistics for dominator optimizations. */
struct opt_stats_d
{
@@ -585,6 +580,45 @@ record_edge_info (basic_block bb)
}
}
+class domfixups : public propagate_cleanups
+{
+public:
+ domfixups (function *fun) : m_fun (fun) { }
+ ~domfixups ();
+private:
+ function *m_fun;
+};
+
+// Mark EH forwarded blocks for cleanup after pass is done.
+
+domfixups::~domfixups ()
+{
+ if (!bitmap_empty_p (m_bbs_fixups))
+ {
+ unsigned i;
+ bitmap_iterator bi;
+
+ /* Jump threading may have created forwarder blocks from blocks
+ needing EH cleanup; the new successor of these blocks, which
+ has inherited from the original block, needs the cleanup.
+ Don't clear bits in the bitmap, as that can break the bitmap
+ iterator. */
+ EXECUTE_IF_SET_IN_BITMAP (m_bbs_fixups, 0, i, bi)
+ {
+ basic_block bb = BASIC_BLOCK_FOR_FN (m_fun, i);
+ if (bb == NULL)
+ continue;
+ while (single_succ_p (bb)
+ && (single_succ_edge (bb)->flags
+ & (EDGE_EH|EDGE_DFS_BACK)) == 0)
+ bb = single_succ (bb);
+ if (bb == EXIT_BLOCK_PTR_FOR_FN (m_fun))
+ continue;
+ if ((unsigned) bb->index != i)
+ record_eh_change (bb);
+ }
+ }
+}
class dom_opt_dom_walker : public dom_walker
{
@@ -592,11 +626,13 @@ public:
dom_opt_dom_walker (cdi_direction direction,
class const_and_copies *const_and_copies,
class avail_exprs_stack *avail_exprs_stack,
- gcond *dummy_cond)
+ gcond *dummy_cond,
+ function *fun)
: dom_walker (direction, REACHABLE_BLOCKS),
m_const_and_copies (const_and_copies),
m_avail_exprs_stack (avail_exprs_stack),
evrp_range_analyzer (true),
+ m_fixups (fun),
m_dummy_cond (dummy_cond) { }
virtual edge before_dom_children (basic_block);
@@ -611,6 +647,8 @@ private:
/* VRP data. */
class evrp_range_analyzer evrp_range_analyzer;
+ domfixups m_fixups;
+
/* Dummy condition to avoid creating lots of throw away statements. */
gcond *m_dummy_cond;
@@ -679,8 +717,6 @@ pass_dominator::execute (function *fun)
class avail_exprs_stack *avail_exprs_stack
= new class avail_exprs_stack (avail_exprs);
class const_and_copies *const_and_copies = new class const_and_copies ();
- need_eh_cleanup = BITMAP_ALLOC (NULL);
- need_noreturn_fixup.create (0);
calculate_dominance_info (CDI_DOMINATORS);
cfg_altered = false;
@@ -720,7 +756,7 @@ pass_dominator::execute (function *fun)
/* Recursively walk the dominator tree optimizing statements. */
dom_opt_dom_walker walker (CDI_DOMINATORS, const_and_copies,
- avail_exprs_stack, dummy_cond);
+ avail_exprs_stack, dummy_cond, fun);
walker.walk (fun->cfg->x_entry_block_ptr);
/* Look for blocks where we cleared EDGE_EXECUTABLE on an outgoing
@@ -778,54 +814,6 @@ pass_dominator::execute (function *fun)
if (cfg_altered)
free_dominance_info (CDI_DOMINATORS);
- /* Removal of statements may make some EH edges dead. Purge
- such edges from the CFG as needed. */
- if (!bitmap_empty_p (need_eh_cleanup))
- {
- unsigned i;
- bitmap_iterator bi;
-
- /* Jump threading may have created forwarder blocks from blocks
- needing EH cleanup; the new successor of these blocks, which
- has inherited from the original block, needs the cleanup.
- Don't clear bits in the bitmap, as that can break the bitmap
- iterator. */
- EXECUTE_IF_SET_IN_BITMAP (need_eh_cleanup, 0, i, bi)
- {
- basic_block bb = BASIC_BLOCK_FOR_FN (fun, i);
- if (bb == NULL)
- continue;
- while (single_succ_p (bb)
- && (single_succ_edge (bb)->flags
- & (EDGE_EH|EDGE_DFS_BACK)) == 0)
- bb = single_succ (bb);
- if (bb == EXIT_BLOCK_PTR_FOR_FN (fun))
- continue;
- if ((unsigned) bb->index != i)
- bitmap_set_bit (need_eh_cleanup, bb->index);
- }
-
- gimple_purge_all_dead_eh_edges (need_eh_cleanup);
- bitmap_clear (need_eh_cleanup);
- }
-
- /* Fixup stmts that became noreturn calls. This may require splitting
- blocks and thus isn't possible during the dominator walk or before
- jump threading finished. Do this in reverse order so we don't
- inadvertedly remove a stmt we want to fixup by visiting a dominating
- now noreturn call first. */
- while (!need_noreturn_fixup.is_empty ())
- {
- gimple *stmt = need_noreturn_fixup.pop ();
- if (dump_file && dump_flags & TDF_DETAILS)
- {
- fprintf (dump_file, "Fixing up noreturn call ");
- print_gimple_stmt (dump_file, stmt, 0);
- fprintf (dump_file, "\n");
- }
- fixup_noreturn_call (stmt);
- }
-
statistics_counter_event (fun, "Redundant expressions eliminated",
opt_stats.num_re);
statistics_counter_event (fun, "Constants propagated",
@@ -844,8 +832,6 @@ pass_dominator::execute (function *fun)
avail_exprs = NULL;
/* Free asserted bitmaps and stacks. */
- BITMAP_FREE (need_eh_cleanup);
- need_noreturn_fixup.release ();
delete avail_exprs_stack;
delete const_and_copies;
@@ -1995,11 +1981,10 @@ dom_opt_dom_walker::optimize_stmt (basic_block bb, gimple_stmt_iterator *si,
gimple *stmt, *old_stmt;
bool may_optimize_p;
bool modified_p = false;
- bool was_noreturn;
edge retval = NULL;
old_stmt = stmt = gsi_stmt (*si);
- was_noreturn = is_gimple_call (stmt) && gimple_call_noreturn_p (stmt);
+ m_fixups.mark_stmt (old_stmt);
if (dump_file && (dump_flags & TDF_DETAILS))
{
@@ -2159,7 +2144,7 @@ dom_opt_dom_walker::optimize_stmt (basic_block bb, gimple_stmt_iterator *si,
unlink_stmt_vdef (stmt);
if (gsi_remove (si, true))
{
- bitmap_set_bit (need_eh_cleanup, bb->index);
+ m_fixups.record_eh_change (bb);
if (dump_file && (dump_flags & TDF_DETAILS))
fprintf (dump_file, " Flagged to clear EH edges.\n");
}
@@ -2218,18 +2203,7 @@ dom_opt_dom_walker::optimize_stmt (basic_block bb, gimple_stmt_iterator *si,
update_stmt_if_modified (stmt);
- /* If we simplified a statement in such a way as to be shown that it
- cannot trap, update the eh information and the cfg to match. */
- if (maybe_clean_or_replace_eh_stmt (old_stmt, stmt))
- {
- bitmap_set_bit (need_eh_cleanup, bb->index);
- if (dump_file && (dump_flags & TDF_DETAILS))
- fprintf (dump_file, " Flagged to clear EH edges.\n");
- }
-
- if (!was_noreturn
- && is_gimple_call (stmt) && gimple_call_noreturn_p (stmt))
- need_noreturn_fixup.safe_push (stmt);
+ m_fixups.record_change (stmt, /*recompute_invariants=*/false);
}
return retval;
}
diff --git a/gcc/tree-ssa-forwprop.c b/gcc/tree-ssa-forwprop.c
index 7dd1e64335a..8007ff50fbf 100644
--- a/gcc/tree-ssa-forwprop.c
+++ b/gcc/tree-ssa-forwprop.c
@@ -629,20 +629,6 @@ forward_propagate_into_cond (gimple_stmt_iterator *gsi_p)
return 0;
}
-/* We've just substituted an ADDR_EXPR into stmt. Update all the
- relevant data structures to match. */
-
-static void
-tidy_after_forward_propagate_addr (gimple *stmt)
-{
- /* We may have turned a trapping insn into a non-trapping insn. */
- if (maybe_clean_or_replace_eh_stmt (stmt, stmt))
- bitmap_set_bit (to_purge, gimple_bb (stmt)->index);
-
- if (TREE_CODE (gimple_assign_rhs1 (stmt)) == ADDR_EXPR)
- recompute_tree_invariant_for_addr_expr (gimple_assign_rhs1 (stmt));
-}
-
/* NAME is a SSA_NAME representing DEF_RHS which is of the form
ADDR_EXPR <whatever>.
@@ -778,7 +764,8 @@ forward_propagate_addr_expr_1 (tree name, tree def_rhs,
TREE_OPERAND (lhs, 0) = new_ptr;
TREE_OPERAND (lhs, 1)
= wide_int_to_tree (TREE_TYPE (TREE_OPERAND (lhs, 1)), off);
- tidy_after_forward_propagate_addr (use_stmt);
+ propagate_mark_stmt_for_cleanup (use_stmt, use_stmt,
+ to_purge, NULL, false);
/* Continue propagating into the RHS if this was not the only use. */
if (single_use_p)
return true;
@@ -824,7 +811,8 @@ forward_propagate_addr_expr_1 (tree name, tree def_rhs,
TREE_THIS_VOLATILE (new_lhs) = TREE_THIS_VOLATILE (lhs);
TREE_SIDE_EFFECTS (new_lhs) = TREE_SIDE_EFFECTS (lhs);
*def_rhs_basep = saved;
- tidy_after_forward_propagate_addr (use_stmt);
+ propagate_mark_stmt_for_cleanup (use_stmt, use_stmt, to_purge,
+ NULL, false);
/* Continue propagating into the RHS if this was not the
only use. */
if (single_use_p)
@@ -870,7 +858,8 @@ forward_propagate_addr_expr_1 (tree name, tree def_rhs,
TREE_OPERAND (rhs, 1)
= wide_int_to_tree (TREE_TYPE (TREE_OPERAND (rhs, 1)), off);
fold_stmt_inplace (use_stmt_gsi);
- tidy_after_forward_propagate_addr (use_stmt);
+ propagate_mark_stmt_for_cleanup (use_stmt, use_stmt, to_purge,
+ NULL, false);
return res;
}
/* If the RHS is a plain dereference and the value type is the same as
@@ -911,7 +900,8 @@ forward_propagate_addr_expr_1 (tree name, tree def_rhs,
TREE_SIDE_EFFECTS (new_rhs) = TREE_SIDE_EFFECTS (rhs);
*def_rhs_basep = saved;
fold_stmt_inplace (use_stmt_gsi);
- tidy_after_forward_propagate_addr (use_stmt);
+ propagate_mark_stmt_for_cleanup (use_stmt, use_stmt, to_purge,
+ NULL, false);
return res;
}
}
@@ -947,7 +937,8 @@ forward_propagate_addr_expr_1 (tree name, tree def_rhs,
gimple_assign_set_rhs_from_tree (use_stmt_gsi, new_rhs);
use_stmt = gsi_stmt (*use_stmt_gsi);
update_stmt (use_stmt);
- tidy_after_forward_propagate_addr (use_stmt);
+ propagate_mark_stmt_for_cleanup (use_stmt, use_stmt, to_purge,
+ NULL, false);
return true;
}
@@ -2625,8 +2616,6 @@ pass_forwprop::execute (function *fun)
gimple *stmt = gsi_stmt (gsi);
gimple *orig_stmt = stmt;
bool changed = false;
- bool was_noreturn = (is_gimple_call (stmt)
- && gimple_call_noreturn_p (stmt));
/* Mark stmt as potentially needing revisiting. */
gimple_set_plf (stmt, GF_PLF_1, false);
@@ -2635,11 +2624,9 @@ pass_forwprop::execute (function *fun)
{
changed = true;
stmt = gsi_stmt (gsi);
- if (maybe_clean_or_replace_eh_stmt (orig_stmt, stmt))
- bitmap_set_bit (to_purge, bb->index);
- if (!was_noreturn
- && is_gimple_call (stmt) && gimple_call_noreturn_p (stmt))
- to_fixup.safe_push (stmt);
+ propagate_mark_stmt_for_cleanup (orig_stmt, stmt,
+ to_purge, &to_fixup, false,
+ /*recompute_inv=*/false);
/* Cleanup the CFG if we simplified a condition to
true or false. */
if (gcond *cond = dyn_cast <gcond *> (stmt))
@@ -2762,23 +2749,8 @@ pass_forwprop::execute (function *fun)
free (postorder);
lattice.release ();
- /* Fixup stmts that became noreturn calls. This may require splitting
- blocks and thus isn't possible during the walk. Do this
- in reverse order so we don't inadvertedly remove a stmt we want to
- fixup by visiting a dominating now noreturn call first. */
- while (!to_fixup.is_empty ())
- {
- gimple *stmt = to_fixup.pop ();
- if (dump_file && dump_flags & TDF_DETAILS)
- {
- fprintf (dump_file, "Fixing up noreturn call ");
- print_gimple_stmt (dump_file, stmt, 0);
- fprintf (dump_file, "\n");
- }
- cfg_changed |= fixup_noreturn_call (stmt);
- }
+ cfg_changed |= propagate_cleanup (to_purge, &to_fixup);
- cfg_changed |= gimple_purge_all_dead_eh_edges (to_purge);
BITMAP_FREE (to_purge);
if (cfg_changed)
diff --git a/gcc/tree-ssa-propagate.c b/gcc/tree-ssa-propagate.c
index 6b78dc1c06c..85b45c923c7 100644
--- a/gcc/tree-ssa-propagate.c
+++ b/gcc/tree-ssa-propagate.c
@@ -38,6 +38,7 @@
#include "cfgloop.h"
#include "tree-cfgcleanup.h"
#include "cfganal.h"
+#include "tree-pass.h"
/* This file implements a generic value propagation engine based on
the same propagation used by the SSA-CCP algorithm [1].
@@ -975,14 +976,10 @@ public:
substitute_and_fold_engine (engine)
{
stmts_to_remove.create (0);
- stmts_to_fixup.create (0);
- need_eh_cleanup = BITMAP_ALLOC (NULL);
}
~substitute_and_fold_dom_walker ()
{
stmts_to_remove.release ();
- stmts_to_fixup.release ();
- BITMAP_FREE (need_eh_cleanup);
}
virtual edge before_dom_children (basic_block);
@@ -990,12 +987,137 @@ public:
bool something_changed;
vec<gimple *> stmts_to_remove;
- vec<gimple *> stmts_to_fixup;
- bitmap need_eh_cleanup;
+ propagate_cleanups fixups;
class substitute_and_fold_engine *substitute_and_fold_engine;
};
+propagate_cleanups::~propagate_cleanups ()
+{
+ if (propagate_cleanup (m_bbs_fixups, &m_stmts_fixups))
+ {
+ m_changed = true;
+ if (m_todo_flags)
+ *m_todo_flags |= TODO_cleanup_cfg;
+ }
+}
+
+void
+propagate_cleanups::record_eh_change (basic_block bb)
+{
+ bitmap_set_bit (m_bbs_fixups, bb->index);
+}
+
+// Mark a statement for future EH cleanup.
+
+void
+propagate_cleanups::mark_stmt (gimple *old_stmt)
+{
+ m_old_stmt = old_stmt;
+ m_was_noreturn = (is_gimple_call (old_stmt)
+ && gimple_call_noreturn_p (old_stmt));
+}
+
+// Record that M_OLD_STMT was changed to NEW_STMT and mark the statement
+// for possible cleanup.
+
+void
+propagate_cleanups::record_change (gimple *new_stmt,
+ bool recompute_invariants)
+{
+ propagate_mark_stmt_for_cleanup (m_old_stmt, new_stmt,
+ m_bbs_fixups, &m_stmts_fixups,
+ m_was_noreturn,
+ recompute_invariants);
+}
+
+// Helper function for propagate_cleanups::record_change. Record that
+// OLD_STMT changed to NEW_STMT.
+//
+// Note: Use the propagate_cleanups class intead of accessing this
+// function directly. This is only here to avoid ripping apart
+// tree-ssa-forwprop.c to use the class. Once forwprop is converted,
+// this should be removed.
+
+void
+propagate_mark_stmt_for_cleanup (gimple *old_stmt,
+ gimple *new_stmt,
+ bitmap bbs_needing_eh_cleanup,
+ vec<gimple *> *stmts_that_became_noreturn,
+ bool was_noreturn,
+ bool recompute_invariants)
+{
+ /* If we cleaned up EH information from the statement, remove EH
+ edges. */
+ if (maybe_clean_or_replace_eh_stmt (old_stmt, new_stmt))
+ {
+ basic_block bb = gimple_bb (new_stmt);
+ bitmap_set_bit (bbs_needing_eh_cleanup, bb->index);
+ if (dump_file && (dump_flags & TDF_DETAILS))
+ fprintf (dump_file, " Removed EH side-effects.\n");
+ }
+
+ if (stmts_that_became_noreturn)
+ {
+ /* If we turned a not noreturn call into a noreturn one, schedule it
+ for fixup. */
+ if (!was_noreturn
+ && is_gimple_call (new_stmt)
+ && gimple_call_noreturn_p (new_stmt))
+ stmts_that_became_noreturn->safe_push (new_stmt);
+ }
+
+ if (recompute_invariants && gimple_assign_single_p (new_stmt))
+ {
+ tree rhs = gimple_assign_rhs1 (new_stmt);
+ if (TREE_CODE (rhs) == ADDR_EXPR)
+ recompute_tree_invariant_for_addr_expr (rhs);
+ }
+}
+
+
+// Helper function for propagate_cleanups destructor.
+//
+// Perform any EH cleanups necessary for BBs in
+// BBS_NEEDING_EH_CLEANUP, and fix up any calls in
+// STMTS_THAT_BECAME_NORETURN. Return TRUE if any cleanups were
+// performed.
+//
+// Note: Use the propagate_cleanups class intead of accessing this
+// function directly. This is only here to avoid ripping apart
+// tree-ssa-forwprop.c to use the class. Once forwprop is converted,
+// this should be removed.
+
+bool
+propagate_cleanup (bitmap bbs_needing_eh_cleanup,
+ vec<gimple *> *stmts_that_became_noreturn)
+{
+ bool changed = false;
+ if (bbs_needing_eh_cleanup && !bitmap_empty_p (bbs_needing_eh_cleanup))
+ {
+ gimple_purge_all_dead_eh_edges (bbs_needing_eh_cleanup);
+ changed = true;
+ }
+
+ /* Fixup stmts that became noreturn calls. This may require splitting
+ blocks and thus isn't possible during the dominator walk. Do this
+ in reverse order so we don't inadvertedly remove a stmt we want to
+ fixup by visiting a dominating now noreturn call first. */
+ while (!stmts_that_became_noreturn->is_empty ())
+ {
+ gimple *stmt = stmts_that_became_noreturn->pop ();
+ if (dump_file && dump_flags & TDF_DETAILS)
+ {
+ fprintf (dump_file, "Fixing up noreturn call ");
+ print_gimple_stmt (dump_file, stmt, 0);
+ fprintf (dump_file, "\n");
+ }
+ if (fixup_noreturn_call (stmt))
+ changed = true;
+ }
+ return changed;
+}
+
edge
substitute_and_fold_dom_walker::before_dom_children (basic_block bb)
{
@@ -1061,8 +1183,7 @@ substitute_and_fold_dom_walker::before_dom_children (basic_block bb)
}
gimple *old_stmt = stmt;
- bool was_noreturn = (is_gimple_call (stmt)
- && gimple_call_noreturn_p (stmt));
+ fixups.mark_stmt (old_stmt);
/* Replace real uses in the statement. */
did_replace |= substitute_and_fold_engine->replace_uses_in (stmt);
@@ -1110,25 +1231,7 @@ substitute_and_fold_dom_walker::before_dom_children (basic_block bb)
/* Now cleanup. */
if (did_replace)
{
- /* If we cleaned up EH information from the statement,
- remove EH edges. */
- if (maybe_clean_or_replace_eh_stmt (old_stmt, stmt))
- bitmap_set_bit (need_eh_cleanup, bb->index);
-
- /* If we turned a not noreturn call into a noreturn one
- schedule it for fixup. */
- if (!was_noreturn
- && is_gimple_call (stmt)
- && gimple_call_noreturn_p (stmt))
- stmts_to_fixup.safe_push (stmt);
-
- if (gimple_assign_single_p (stmt))
- {
- tree rhs = gimple_assign_rhs1 (stmt);
-
- if (TREE_CODE (rhs) == ADDR_EXPR)
- recompute_tree_invariant_for_addr_expr (rhs);
- }
+ fixups.record_change (stmt);
/* Determine what needs to be done to update the SSA form. */
update_stmt_if_modified (stmt);
@@ -1217,25 +1320,6 @@ substitute_and_fold_engine::substitute_and_fold (basic_block block)
}
}
- if (!bitmap_empty_p (walker.need_eh_cleanup))
- gimple_purge_all_dead_eh_edges (walker.need_eh_cleanup);
-
- /* Fixup stmts that became noreturn calls. This may require splitting
- blocks and thus isn't possible during the dominator walk. Do this
- in reverse order so we don't inadvertedly remove a stmt we want to
- fixup by visiting a dominating now noreturn call first. */
- while (!walker.stmts_to_fixup.is_empty ())
- {
- gimple *stmt = walker.stmts_to_fixup.pop ();
- if (dump_file && dump_flags & TDF_DETAILS)
- {
- fprintf (dump_file, "Fixing up noreturn call ");
- print_gimple_stmt (dump_file, stmt, 0);
- fprintf (dump_file, "\n");
- }
- fixup_noreturn_call (stmt);
- }
-
statistics_counter_event (cfun, "Constants propagated",
prop_stats.num_const_prop);
statistics_counter_event (cfun, "Copies propagated",
diff --git a/gcc/tree-ssa-propagate.h b/gcc/tree-ssa-propagate.h
index 81b635e0787..02c120a5fd7 100644
--- a/gcc/tree-ssa-propagate.h
+++ b/gcc/tree-ssa-propagate.h
@@ -109,4 +109,45 @@ class substitute_and_fold_engine
bool replace_phi_args_in (gphi *);
};
+// Class to record statement propagation changes. Upon destruction of
+// this class, any possible cleanups to EH are performed.
+
+class propagate_cleanups
+{
+public:
+ propagate_cleanups (unsigned int *flags = NULL) : m_todo_flags (flags) { }
+ ~propagate_cleanups ();
+ void mark_stmt (gimple *);
+ void record_change (gimple *, bool recompute_invariants = true);
+ void record_eh_change (basic_block);
+
+protected:
+ // Basic blocks needing EH fixups.
+ auto_bitmap m_bbs_fixups;
+private:
+ // Statements that became `noreturn' and need fixups.
+ auto_vec<gimple *> m_stmts_fixups;
+ // Set if any cleanups were performed.
+ bool m_changed;
+ // If non-NULL and cleanups were performed, this will contain any
+ // TODO_flags that should be performed after the destructor completes.
+ unsigned int *m_todo_flags;
+ gimple *m_old_stmt;
+ // Old statement had noreturn attribute.
+ bool m_was_noreturn;
+};
+
+// Note: Use the above class intead of these functions. The only
+// reason these are still here are to avoid ripping apart
+// tree-ssa-forwprop.c to use the class. Once forwprop is converted,
+// these should be removed.
+extern void propagate_mark_stmt_for_cleanup
+ (gimple *old_stmt, gimple *new_stmt,
+ bitmap bbs_needing_eh_cleanup,
+ vec<gimple *> *stmts_that_became_noreturn,
+ bool was_noreturn,
+ bool recompute_invariants = true);
+bool propagate_cleanup (bitmap bbs_needing_eh_cleanup,
+ vec<gimple *> *stmts_that_became_noreturn);
+
#endif /* _TREE_SSA_PROPAGATE_H */
diff --git a/gcc/tree-ssa-sccvn.c b/gcc/tree-ssa-sccvn.c
index a174f18f72a..70662be96b8 100644
--- a/gcc/tree-ssa-sccvn.c
+++ b/gcc/tree-ssa-sccvn.c
@@ -70,6 +70,7 @@ along with GCC; see the file COPYING3. If not see
#include "tree-scalar-evolution.h"
#include "tree-ssa-loop-niter.h"
#include "tree-ssa-sccvn.h"
+#include "tree-ssa-propagate.h"
/* This algorithm is based on the SCC algorithm presented by Keith
Cooper and L. Taylor Simpson in "SCC-Based Value numbering"
@@ -1870,7 +1871,7 @@ vn_nary_simplify (vn_nary_op_t nary)
class eliminate_dom_walker : public dom_walker
{
public:
- eliminate_dom_walker (cdi_direction, bitmap);
+ eliminate_dom_walker (cdi_direction, bitmap, unsigned *flags);
~eliminate_dom_walker ();
virtual edge before_dom_children (basic_block);
@@ -1889,18 +1890,17 @@ public:
unsigned int eliminations;
unsigned int insertions;
+ /* Post propagation cleanups. */
+ propagate_cleanups fixups;
+
/* SSA names that had their defs inserted by PRE if do_pre. */
bitmap inserted_exprs;
- /* Blocks with statements that have had their EH properties changed. */
- bitmap need_eh_cleanup;
-
/* Blocks with statements that have had their AB properties changed. */
bitmap need_ab_cleanup;
/* Local state for the eliminate domwalk. */
auto_vec<gimple *> to_remove;
- auto_vec<gimple *> to_fixup;
auto_vec<tree> avail;
auto_vec<tree> avail_stack;
};
@@ -1911,7 +1911,7 @@ class rpo_elim : public eliminate_dom_walker
{
public:
rpo_elim(basic_block entry_)
- : eliminate_dom_walker (CDI_DOMINATORS, NULL), entry (entry_) {}
+ : eliminate_dom_walker (CDI_DOMINATORS, NULL, NULL), entry (entry_) {}
~rpo_elim();
virtual tree eliminate_avail (basic_block, tree op);
@@ -4818,18 +4818,18 @@ vn_reference_may_trap (vn_reference_t ref)
}
eliminate_dom_walker::eliminate_dom_walker (cdi_direction direction,
- bitmap inserted_exprs_)
+ bitmap inserted_exprs_,
+ unsigned *flags)
: dom_walker (direction), do_pre (inserted_exprs_ != NULL),
el_todo (0), eliminations (0), insertions (0),
+ fixups (flags),
inserted_exprs (inserted_exprs_)
{
- need_eh_cleanup = BITMAP_ALLOC (NULL);
need_ab_cleanup = BITMAP_ALLOC (NULL);
}
eliminate_dom_walker::~eliminate_dom_walker ()
{
- BITMAP_FREE (need_eh_cleanup);
BITMAP_FREE (need_ab_cleanup);
}
@@ -5152,8 +5152,7 @@ eliminate_dom_walker::eliminate_stmt (basic_block b, gimple_stmt_iterator *gsi)
its EH information. */
if (maybe_clean_or_replace_eh_stmt (orig_stmt, stmt))
{
- bitmap_set_bit (need_eh_cleanup,
- gimple_bb (stmt)->index);
+ fixups.record_eh_change (gimple_bb (stmt));
if (dump_file && (dump_flags & TDF_DETAILS))
fprintf (dump_file, " Removed EH side-effects.\n");
}
@@ -5237,8 +5236,6 @@ eliminate_dom_walker::eliminate_stmt (basic_block b, gimple_stmt_iterator *gsi)
}
bool can_make_abnormal_goto = stmt_can_make_abnormal_goto (stmt);
- bool was_noreturn = (is_gimple_call (stmt)
- && gimple_call_noreturn_p (stmt));
tree vdef = gimple_vdef (stmt);
tree vuse = gimple_vuse (stmt);
@@ -5286,6 +5283,7 @@ eliminate_dom_walker::eliminate_stmt (basic_block b, gimple_stmt_iterator *gsi)
/* Fold the stmt if modified, this canonicalizes MEM_REFs we propagated
into which is a requirement for the IPA devirt machinery. */
gimple *old_stmt = stmt;
+ fixups.mark_stmt (old_stmt);
if (modified)
{
/* If a formerly non-invariant ADDR_EXPR is turned into an
@@ -5391,11 +5389,7 @@ eliminate_dom_walker::eliminate_stmt (basic_block b, gimple_stmt_iterator *gsi)
if (modified)
{
- /* When changing a call into a noreturn call, cfg cleanup
- is needed to fix up the noreturn call. */
- if (!was_noreturn
- && is_gimple_call (stmt) && gimple_call_noreturn_p (stmt))
- to_fixup.safe_push (stmt);
+ fixups.record_change (stmt);
/* When changing a condition or switch into one we know what
edge will be executed, schedule a cfg cleanup. */
if ((gimple_code (stmt) == GIMPLE_COND
@@ -5405,16 +5399,8 @@ eliminate_dom_walker::eliminate_stmt (basic_block b, gimple_stmt_iterator *gsi)
&& TREE_CODE (gimple_switch_index
(as_a <gswitch *> (stmt))) == INTEGER_CST))
el_todo |= TODO_cleanup_cfg;
- /* If we removed EH side-effects from the statement, clean
- its EH information. */
- if (maybe_clean_or_replace_eh_stmt (old_stmt, stmt))
- {
- bitmap_set_bit (need_eh_cleanup,
- gimple_bb (stmt)->index);
- if (dump_file && (dump_flags & TDF_DETAILS))
- fprintf (dump_file, " Removed EH side-effects.\n");
- }
- /* Likewise for AB side-effects. */
+ /* If we removed AB side-effects from the statement, clean its
+ AB call edges. */
if (can_make_abnormal_goto
&& !stmt_can_make_abnormal_goto (stmt))
{
@@ -5609,7 +5595,7 @@ eliminate_dom_walker::eliminate_cleanup (bool region_p)
stmt = gsi_stmt (gsi);
update_stmt (stmt);
if (maybe_clean_or_replace_eh_stmt (stmt, stmt))
- bitmap_set_bit (need_eh_cleanup, gimple_bb (stmt)->index);
+ fixups.record_eh_change (gimple_bb (stmt));
continue;
}
else
@@ -5635,7 +5621,7 @@ eliminate_dom_walker::eliminate_cleanup (bool region_p)
basic_block bb = gimple_bb (stmt);
unlink_stmt_vdef (stmt);
if (gsi_remove (&gsi, true))
- bitmap_set_bit (need_eh_cleanup, bb->index);
+ fixups.record_eh_change (bb);
if (is_gimple_call (stmt) && stmt_can_make_abnormal_goto (stmt))
bitmap_set_bit (need_ab_cleanup, bb->index);
if (do_release_defs)
@@ -5646,36 +5632,12 @@ eliminate_dom_walker::eliminate_cleanup (bool region_p)
el_todo |= TODO_cleanup_cfg;
}
- /* Fixup stmts that became noreturn calls. This may require splitting
- blocks and thus isn't possible during the dominator walk. Do this
- in reverse order so we don't inadvertedly remove a stmt we want to
- fixup by visiting a dominating now noreturn call first. */
- while (!to_fixup.is_empty ())
+ if (!bitmap_empty_p (need_ab_cleanup))
{
- gimple *stmt = to_fixup.pop ();
-
- if (dump_file && (dump_flags & TDF_DETAILS))
- {
- fprintf (dump_file, "Fixing up noreturn call ");
- print_gimple_stmt (dump_file, stmt, 0);
- }
-
- if (fixup_noreturn_call (stmt))
- el_todo |= TODO_cleanup_cfg;
+ gimple_purge_all_dead_abnormal_call_edges (need_ab_cleanup);
+ el_todo |= TODO_cleanup_cfg;
}
- bool do_eh_cleanup = !bitmap_empty_p (need_eh_cleanup);
- bool do_ab_cleanup = !bitmap_empty_p (need_ab_cleanup);
-
- if (do_eh_cleanup)
- gimple_purge_all_dead_eh_edges (need_eh_cleanup);
-
- if (do_ab_cleanup)
- gimple_purge_all_dead_abnormal_call_edges (need_ab_cleanup);
-
- if (do_eh_cleanup || do_ab_cleanup)
- el_todo |= TODO_cleanup_cfg;
-
return el_todo;
}
@@ -5684,10 +5646,13 @@ eliminate_dom_walker::eliminate_cleanup (bool region_p)
unsigned
eliminate_with_rpo_vn (bitmap inserted_exprs)
{
- eliminate_dom_walker walker (CDI_DOMINATORS, inserted_exprs);
-
- walker.walk (cfun->cfg->x_entry_block_ptr);
- return walker.eliminate_cleanup ();
+ unsigned flags = 0;
+ {
+ eliminate_dom_walker walker (CDI_DOMINATORS, inserted_exprs, &flags);
+ walker.walk (cfun->cfg->x_entry_block_ptr);
+ flags |= walker.eliminate_cleanup ();
+ }
+ return flags;
}
static unsigned
