Sorry, I meant to PING this one.
Aldy
On Wed, May 8, 2019 at 5:08 PM Aldy Hernandez <al...@redhat.com> wrote:
>
> 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
