abstract out EH propagation cleanups

[Aldy Hernandez]

PING

On Wed, May 8, 2019 at 5:18 PM Aldy Hernandez <aldyh@redhat.com> wrote:
>
>
>
> On 5/8/19 2:30 AM, Richard Biener wrote:
> > On Tue, May 7, 2019 at 11:55 PM Jeff Law <law@redhat.com> wrote:
> >>
> >> On 5/7/19 3:45 AM, Richard Biener wrote:
> >>> On Tue, May 7, 2019 at 11:13 AM Aldy Hernandez <aldyh@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.
> >
> > 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.
> >
> > 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).
> >
> > 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
>
> BTW, in case it wasn't clear.  I do understand that not all copies are
> identical, that is why the interface has the recompute_invariants and
> m_todo_flags fields.  It is *supposed* to work as before, and it if
> isn't, it's a bug and I'll gladly fix it.
>
> Aldy