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.
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).
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.
Richard.
> Aldy
>
> (*) If this is too invasive for the period immediately following the
> re-opening of stage1, I can hold off the commit (if approved).
