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

Reply via email to