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
