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