| Message ID | f8f57d04-7159-4e3e-4f66-9e073a5ac01c@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | abstract out EH propagation cleanups | expand |
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. 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).
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. > > 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
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
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. 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
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
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
Sorry, I meant to PING this one. Aldy On Wed, May 8, 2019 at 5:08 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. > > 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
PING*2 On 5/15/19 12:36 PM, Aldy Hernandez wrote: > Sorry, I meant to PING this one. > > Aldy > > On Wed, May 8, 2019 at 5:08 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. >> >> 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
gcc/ * tree-ssa-propagate.h (class propagate_cleanups): New. * tree-ssa-propagate.c (class substitute_and_fold_dom_walker): Remove references to stmts_to_fixup and need_eh_cleanup. (propagate_cleanups::~propagate_cleanups): New. (propagate_cleanups::record_eh_change): New. (propagate_mark_stmt_for_cleanup): New. (propagate_cleanup): New. (substitute_and_fold_dom_walker::before_dom_children): Adjust for propagate_cleanups class. (substitute_and_fold_engine::substitute_and_fold): Remove manual EH cleanups. * gimple-ssa-evrp.c (class evrp_dom_walker): Adjust for propagate_cleanups class. (evrp_dom_walker::before_dom_children): Same. (evrp_dom_walker::cleanup): Same. * tree-ssa-dom.c: Remove need_eh_cleanup and need_noreturn_fixup globals. (class domfixups): New. (class dom_opt_dom_walker): Adjust for propagate_cleanups class. (pass_dominator::execute): Same. (dom_opt_dom_walker::optimize_stmt): Same. * tree-ssa-forwprop.c (tidy_after_forward_propagate_addr): Remove. (forward_propagate_addr_expr_1): Call propagate_mark_stmt_for_cleanup instead of tidy_after_forward_propagate_addr. (pass_forwprop::execute): Adjust for propagate_cleanups class. * tree-ssa-sccvn.c (class eliminate_dom_walker): Same. (class rpo_elim): Same. (eliminate_dom_walker::eliminate_dom_walker): Same. (eliminate_dom_walker::~eliminate_dom_walker): Same. (eliminate_dom_walker::eliminate_stmt): Same. (eliminate_dom_walker::eliminate_cleanup): Same. (eliminate_with_rpo_vn): Same. diff --git a/gcc/gimple-ssa-evrp.c b/gcc/gimple-ssa-evrp.c index 96da79bf028..ec3fa3a0918 100644 --- a/gcc/gimple-ssa-evrp.c +++ b/gcc/gimple-ssa-evrp.c @@ -73,11 +73,9 @@ public: evrp_range_analyzer (true), evrp_folder (evrp_range_analyzer.get_vr_values ()) { - need_eh_cleanup = BITMAP_ALLOC (NULL); } ~evrp_dom_walker () { - BITMAP_FREE (need_eh_cleanup); } virtual edge before_dom_children (basic_block); virtual void after_dom_children (basic_block); @@ -85,9 +83,8 @@ public: private: DISABLE_COPY_AND_ASSIGN (evrp_dom_walker); - bitmap need_eh_cleanup; - auto_vec<gimple *> stmts_to_fixup; auto_vec<gimple *> stmts_to_remove; + propagate_cleanups fixups; class evrp_range_analyzer evrp_range_analyzer; class evrp_folder evrp_folder; @@ -128,8 +125,6 @@ evrp_dom_walker::before_dom_children (basic_block bb) gimple *stmt = gsi_stmt (gsi); tree output = NULL_TREE; gimple *old_stmt = stmt; - bool was_noreturn = (is_gimple_call (stmt) - && gimple_call_noreturn_p (stmt)); if (dump_file && (dump_flags & TDF_DETAILS)) { @@ -190,26 +185,7 @@ evrp_dom_walker::before_dom_children (basic_block bb) } if (did_replace) - { - /* 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); - } - } + fixups.record_change (old_stmt, stmt); } /* Visit BB successor PHI nodes and replace PHI args. */ @@ -275,19 +251,6 @@ evrp_dom_walker::cleanup (void) } } - 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); - } - evrp_folder.vr_values->cleanup_edges_and_switches (); } diff --git a/gcc/tree-ssa-dom.c b/gcc/tree-ssa-dom.c index b0d56fcf3e3..228f56035b1 100644 --- a/gcc/tree-ssa-dom.c +++ b/gcc/tree-ssa-dom.c @@ -89,11 +89,6 @@ class edge_info /* Track whether or not we have changed the control flow graph. */ static bool cfg_altered; -/* Bitmap of blocks that have had EH statements cleaned. We should - remove their dead edges eventually. */ -static bitmap need_eh_cleanup; -static vec<gimple *> need_noreturn_fixup; - /* Statistics for dominator optimizations. */ struct opt_stats_d { @@ -585,6 +580,45 @@ record_edge_info (basic_block bb) } } +class domfixups : public propagate_cleanups +{ +public: + domfixups (function *fun) : m_fun (fun) { } + ~domfixups (); +private: + function *m_fun; +}; + +// Mark EH forwarded blocks for cleanup after pass is done. + +domfixups::~domfixups () +{ + if (!bitmap_empty_p (m_bbs_fixups)) + { + unsigned i; + bitmap_iterator bi; + + /* Jump threading may have created forwarder blocks from blocks + needing EH cleanup; the new successor of these blocks, which + has inherited from the original block, needs the cleanup. + Don't clear bits in the bitmap, as that can break the bitmap + iterator. */ + EXECUTE_IF_SET_IN_BITMAP (m_bbs_fixups, 0, i, bi) + { + basic_block bb = BASIC_BLOCK_FOR_FN (m_fun, i); + if (bb == NULL) + continue; + while (single_succ_p (bb) + && (single_succ_edge (bb)->flags + & (EDGE_EH|EDGE_DFS_BACK)) == 0) + bb = single_succ (bb); + if (bb == EXIT_BLOCK_PTR_FOR_FN (m_fun)) + continue; + if ((unsigned) bb->index != i) + record_eh_change (bb); + } + } +} class dom_opt_dom_walker : public dom_walker { @@ -592,11 +626,13 @@ public: dom_opt_dom_walker (cdi_direction direction, class const_and_copies *const_and_copies, class avail_exprs_stack *avail_exprs_stack, - gcond *dummy_cond) + gcond *dummy_cond, + function *fun) : dom_walker (direction, REACHABLE_BLOCKS), m_const_and_copies (const_and_copies), m_avail_exprs_stack (avail_exprs_stack), evrp_range_analyzer (true), + m_fixups (fun), m_dummy_cond (dummy_cond) { } virtual edge before_dom_children (basic_block); @@ -611,6 +647,8 @@ private: /* VRP data. */ class evrp_range_analyzer evrp_range_analyzer; + domfixups m_fixups; + /* Dummy condition to avoid creating lots of throw away statements. */ gcond *m_dummy_cond; @@ -679,8 +717,6 @@ pass_dominator::execute (function *fun) class avail_exprs_stack *avail_exprs_stack = new class avail_exprs_stack (avail_exprs); class const_and_copies *const_and_copies = new class const_and_copies (); - need_eh_cleanup = BITMAP_ALLOC (NULL); - need_noreturn_fixup.create (0); calculate_dominance_info (CDI_DOMINATORS); cfg_altered = false; @@ -720,7 +756,7 @@ pass_dominator::execute (function *fun) /* Recursively walk the dominator tree optimizing statements. */ dom_opt_dom_walker walker (CDI_DOMINATORS, const_and_copies, - avail_exprs_stack, dummy_cond); + avail_exprs_stack, dummy_cond, fun); walker.walk (fun->cfg->x_entry_block_ptr); /* Look for blocks where we cleared EDGE_EXECUTABLE on an outgoing @@ -778,54 +814,6 @@ pass_dominator::execute (function *fun) if (cfg_altered) free_dominance_info (CDI_DOMINATORS); - /* Removal of statements may make some EH edges dead. Purge - such edges from the CFG as needed. */ - if (!bitmap_empty_p (need_eh_cleanup)) - { - unsigned i; - bitmap_iterator bi; - - /* Jump threading may have created forwarder blocks from blocks - needing EH cleanup; the new successor of these blocks, which - has inherited from the original block, needs the cleanup. - Don't clear bits in the bitmap, as that can break the bitmap - iterator. */ - EXECUTE_IF_SET_IN_BITMAP (need_eh_cleanup, 0, i, bi) - { - basic_block bb = BASIC_BLOCK_FOR_FN (fun, i); - if (bb == NULL) - continue; - while (single_succ_p (bb) - && (single_succ_edge (bb)->flags - & (EDGE_EH|EDGE_DFS_BACK)) == 0) - bb = single_succ (bb); - if (bb == EXIT_BLOCK_PTR_FOR_FN (fun)) - continue; - if ((unsigned) bb->index != i) - bitmap_set_bit (need_eh_cleanup, bb->index); - } - - gimple_purge_all_dead_eh_edges (need_eh_cleanup); - bitmap_clear (need_eh_cleanup); - } - - /* Fixup stmts that became noreturn calls. This may require splitting - blocks and thus isn't possible during the dominator walk or before - jump threading finished. 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 (!need_noreturn_fixup.is_empty ()) - { - gimple *stmt = need_noreturn_fixup.pop (); - if (dump_file && dump_flags & TDF_DETAILS) - { - fprintf (dump_file, "Fixing up noreturn call "); - print_gimple_stmt (dump_file, stmt, 0); - fprintf (dump_file, "\n"); - } - fixup_noreturn_call (stmt); - } - statistics_counter_event (fun, "Redundant expressions eliminated", opt_stats.num_re); statistics_counter_event (fun, "Constants propagated", @@ -844,8 +832,6 @@ pass_dominator::execute (function *fun) avail_exprs = NULL; /* Free asserted bitmaps and stacks. */ - BITMAP_FREE (need_eh_cleanup); - need_noreturn_fixup.release (); delete avail_exprs_stack; delete const_and_copies; @@ -1995,11 +1981,9 @@ dom_opt_dom_walker::optimize_stmt (basic_block bb, gimple_stmt_iterator *si, gimple *stmt, *old_stmt; bool may_optimize_p; bool modified_p = false; - bool was_noreturn; edge retval = NULL; old_stmt = stmt = gsi_stmt (*si); - was_noreturn = is_gimple_call (stmt) && gimple_call_noreturn_p (stmt); if (dump_file && (dump_flags & TDF_DETAILS)) { @@ -2159,7 +2143,7 @@ dom_opt_dom_walker::optimize_stmt (basic_block bb, gimple_stmt_iterator *si, unlink_stmt_vdef (stmt); if (gsi_remove (si, true)) { - bitmap_set_bit (need_eh_cleanup, bb->index); + m_fixups.record_eh_change (bb); if (dump_file && (dump_flags & TDF_DETAILS)) fprintf (dump_file, " Flagged to clear EH edges.\n"); } @@ -2218,18 +2202,7 @@ dom_opt_dom_walker::optimize_stmt (basic_block bb, gimple_stmt_iterator *si, update_stmt_if_modified (stmt); - /* If we simplified a statement in such a way as to be shown that it - cannot trap, update the eh information and the cfg to match. */ - if (maybe_clean_or_replace_eh_stmt (old_stmt, stmt)) - { - bitmap_set_bit (need_eh_cleanup, bb->index); - if (dump_file && (dump_flags & TDF_DETAILS)) - fprintf (dump_file, " Flagged to clear EH edges.\n"); - } - - if (!was_noreturn - && is_gimple_call (stmt) && gimple_call_noreturn_p (stmt)) - need_noreturn_fixup.safe_push (stmt); + m_fixups.record_change (old_stmt, stmt, /*recompute_invariants=*/false); } return retval; } diff --git a/gcc/tree-ssa-forwprop.c b/gcc/tree-ssa-forwprop.c index 7dd1e64335a..92672327299 100644 --- a/gcc/tree-ssa-forwprop.c +++ b/gcc/tree-ssa-forwprop.c @@ -629,20 +629,6 @@ forward_propagate_into_cond (gimple_stmt_iterator *gsi_p) return 0; } -/* We've just substituted an ADDR_EXPR into stmt. Update all the - relevant data structures to match. */ - -static void -tidy_after_forward_propagate_addr (gimple *stmt) -{ - /* We may have turned a trapping insn into a non-trapping insn. */ - if (maybe_clean_or_replace_eh_stmt (stmt, stmt)) - bitmap_set_bit (to_purge, gimple_bb (stmt)->index); - - if (TREE_CODE (gimple_assign_rhs1 (stmt)) == ADDR_EXPR) - recompute_tree_invariant_for_addr_expr (gimple_assign_rhs1 (stmt)); -} - /* NAME is a SSA_NAME representing DEF_RHS which is of the form ADDR_EXPR <whatever>. @@ -778,7 +764,8 @@ forward_propagate_addr_expr_1 (tree name, tree def_rhs, TREE_OPERAND (lhs, 0) = new_ptr; TREE_OPERAND (lhs, 1) = wide_int_to_tree (TREE_TYPE (TREE_OPERAND (lhs, 1)), off); - tidy_after_forward_propagate_addr (use_stmt); + propagate_mark_stmt_for_cleanup (use_stmt, use_stmt, + to_purge, NULL); /* Continue propagating into the RHS if this was not the only use. */ if (single_use_p) return true; @@ -824,7 +811,8 @@ forward_propagate_addr_expr_1 (tree name, tree def_rhs, TREE_THIS_VOLATILE (new_lhs) = TREE_THIS_VOLATILE (lhs); TREE_SIDE_EFFECTS (new_lhs) = TREE_SIDE_EFFECTS (lhs); *def_rhs_basep = saved; - tidy_after_forward_propagate_addr (use_stmt); + propagate_mark_stmt_for_cleanup (use_stmt, use_stmt, to_purge, + NULL); /* Continue propagating into the RHS if this was not the only use. */ if (single_use_p) @@ -870,7 +858,8 @@ forward_propagate_addr_expr_1 (tree name, tree def_rhs, TREE_OPERAND (rhs, 1) = wide_int_to_tree (TREE_TYPE (TREE_OPERAND (rhs, 1)), off); fold_stmt_inplace (use_stmt_gsi); - tidy_after_forward_propagate_addr (use_stmt); + propagate_mark_stmt_for_cleanup (use_stmt, use_stmt, to_purge, + NULL); return res; } /* If the RHS is a plain dereference and the value type is the same as @@ -911,7 +900,8 @@ forward_propagate_addr_expr_1 (tree name, tree def_rhs, TREE_SIDE_EFFECTS (new_rhs) = TREE_SIDE_EFFECTS (rhs); *def_rhs_basep = saved; fold_stmt_inplace (use_stmt_gsi); - tidy_after_forward_propagate_addr (use_stmt); + propagate_mark_stmt_for_cleanup (use_stmt, use_stmt, to_purge, + NULL); return res; } } @@ -947,7 +937,7 @@ forward_propagate_addr_expr_1 (tree name, tree def_rhs, gimple_assign_set_rhs_from_tree (use_stmt_gsi, new_rhs); use_stmt = gsi_stmt (*use_stmt_gsi); update_stmt (use_stmt); - tidy_after_forward_propagate_addr (use_stmt); + propagate_mark_stmt_for_cleanup (use_stmt, use_stmt, to_purge, NULL); return true; } @@ -2625,8 +2615,6 @@ pass_forwprop::execute (function *fun) gimple *stmt = gsi_stmt (gsi); gimple *orig_stmt = stmt; bool changed = false; - bool was_noreturn = (is_gimple_call (stmt) - && gimple_call_noreturn_p (stmt)); /* Mark stmt as potentially needing revisiting. */ gimple_set_plf (stmt, GF_PLF_1, false); @@ -2635,11 +2623,9 @@ pass_forwprop::execute (function *fun) { changed = true; stmt = gsi_stmt (gsi); - if (maybe_clean_or_replace_eh_stmt (orig_stmt, stmt)) - bitmap_set_bit (to_purge, bb->index); - if (!was_noreturn - && is_gimple_call (stmt) && gimple_call_noreturn_p (stmt)) - to_fixup.safe_push (stmt); + propagate_mark_stmt_for_cleanup (orig_stmt, stmt, + to_purge, &to_fixup, + /*recompute_inv=*/false); /* Cleanup the CFG if we simplified a condition to true or false. */ if (gcond *cond = dyn_cast <gcond *> (stmt)) @@ -2762,23 +2748,8 @@ pass_forwprop::execute (function *fun) free (postorder); lattice.release (); - /* Fixup stmts that became noreturn calls. This may require splitting - blocks and thus isn't possible during the 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 (!to_fixup.is_empty ()) - { - gimple *stmt = to_fixup.pop (); - if (dump_file && dump_flags & TDF_DETAILS) - { - fprintf (dump_file, "Fixing up noreturn call "); - print_gimple_stmt (dump_file, stmt, 0); - fprintf (dump_file, "\n"); - } - cfg_changed |= fixup_noreturn_call (stmt); - } + cfg_changed |= propagate_cleanup (to_purge, &to_fixup); - cfg_changed |= gimple_purge_all_dead_eh_edges (to_purge); BITMAP_FREE (to_purge); if (cfg_changed) diff --git a/gcc/tree-ssa-propagate.c b/gcc/tree-ssa-propagate.c index 6b78dc1c06c..837dd55187d 100644 --- a/gcc/tree-ssa-propagate.c +++ b/gcc/tree-ssa-propagate.c @@ -38,6 +38,7 @@ #include "cfgloop.h" #include "tree-cfgcleanup.h" #include "cfganal.h" +#include "tree-pass.h" /* This file implements a generic value propagation engine based on the same propagation used by the SSA-CCP algorithm [1]. @@ -975,14 +976,10 @@ public: substitute_and_fold_engine (engine) { stmts_to_remove.create (0); - stmts_to_fixup.create (0); - need_eh_cleanup = BITMAP_ALLOC (NULL); } ~substitute_and_fold_dom_walker () { stmts_to_remove.release (); - stmts_to_fixup.release (); - BITMAP_FREE (need_eh_cleanup); } virtual edge before_dom_children (basic_block); @@ -990,12 +987,127 @@ public: bool something_changed; vec<gimple *> stmts_to_remove; - vec<gimple *> stmts_to_fixup; - bitmap need_eh_cleanup; + propagate_cleanups fixups; class substitute_and_fold_engine *substitute_and_fold_engine; }; +propagate_cleanups::~propagate_cleanups () +{ + if (propagate_cleanup (m_bbs_fixups, &m_stmts_fixups)) + { + m_changed = true; + if (m_todo_flags) + *m_todo_flags |= TODO_cleanup_cfg; + } +} + +void +propagate_cleanups::record_eh_change (basic_block bb) +{ + bitmap_set_bit (m_bbs_fixups, bb->index); +} + +// Record that OLD_STMT was changed to NEW_STMT and mark the statement +// for possible cleanup. + +void +propagate_cleanups::record_change (gimple *old_stmt, gimple *new_stmt, + bool recompute_invariants) +{ + propagate_mark_stmt_for_cleanup (old_stmt, new_stmt, + m_bbs_fixups, &m_stmts_fixups, + recompute_invariants); +} + +// Helper function for propagate_cleanups::record_change. Record that +// OLD_STMT changed to NEW_STMT. +// +// Note: Use the propagate_cleanups class intead of accessing this +// function directly. This is only here to avoid ripping apart +// tree-ssa-forwprop.c to use the class. Once forwprop is converted, +// this should be removed. + +void +propagate_mark_stmt_for_cleanup (gimple *old_stmt, + gimple *new_stmt, + bitmap bbs_needing_eh_cleanup, + vec<gimple *> *stmts_that_became_noreturn, + bool recompute_invariants) +{ + /* If we cleaned up EH information from the statement, remove EH + edges. */ + if (maybe_clean_or_replace_eh_stmt (old_stmt, new_stmt)) + { + basic_block bb = gimple_bb (new_stmt); + bitmap_set_bit (bbs_needing_eh_cleanup, bb->index); + if (dump_file && (dump_flags & TDF_DETAILS)) + fprintf (dump_file, " Removed EH side-effects.\n"); + } + + if (stmts_that_became_noreturn) + { + bool was_noreturn = (is_gimple_call (old_stmt) + && gimple_call_noreturn_p (old_stmt)); + /* If we turned a not noreturn call into a noreturn one, schedule it + for fixup. */ + if (!was_noreturn + && is_gimple_call (new_stmt) + && gimple_call_noreturn_p (new_stmt)) + stmts_that_became_noreturn->safe_push (new_stmt); + } + + if (recompute_invariants && gimple_assign_single_p (new_stmt)) + { + tree rhs = gimple_assign_rhs1 (new_stmt); + if (TREE_CODE (rhs) == ADDR_EXPR) + recompute_tree_invariant_for_addr_expr (rhs); + } +} + + +// Helper function for propagate_cleanups destructor. +// +// Perform any EH cleanups necessary for BBs in +// BBS_NEEDING_EH_CLEANUP, and fix up any calls in +// STMTS_THAT_BECAME_NORETURN. Return TRUE if any cleanups were +// performed. +// +// Note: Use the propagate_cleanups class intead of accessing this +// function directly. This is only here to avoid ripping apart +// tree-ssa-forwprop.c to use the class. Once forwprop is converted, +// this should be removed. + +bool +propagate_cleanup (bitmap bbs_needing_eh_cleanup, + vec<gimple *> *stmts_that_became_noreturn) +{ + bool changed = false; + if (bbs_needing_eh_cleanup && !bitmap_empty_p (bbs_needing_eh_cleanup)) + { + gimple_purge_all_dead_eh_edges (bbs_needing_eh_cleanup); + changed = true; + } + + /* 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_that_became_noreturn->is_empty ()) + { + gimple *stmt = stmts_that_became_noreturn->pop (); + if (dump_file && dump_flags & TDF_DETAILS) + { + fprintf (dump_file, "Fixing up noreturn call "); + print_gimple_stmt (dump_file, stmt, 0); + fprintf (dump_file, "\n"); + } + if (fixup_noreturn_call (stmt)) + changed = true; + } + return changed; +} + edge substitute_and_fold_dom_walker::before_dom_children (basic_block bb) { @@ -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) { - /* 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); - } + fixups.record_change (old_stmt, stmt); /* Determine what needs to be done to update the SSA form. */ update_stmt_if_modified (stmt); @@ -1217,25 +1309,6 @@ substitute_and_fold_engine::substitute_and_fold (basic_block block) } } - if (!bitmap_empty_p (walker.need_eh_cleanup)) - gimple_purge_all_dead_eh_edges (walker.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 (!walker.stmts_to_fixup.is_empty ()) - { - gimple *stmt = walker.stmts_to_fixup.pop (); - if (dump_file && dump_flags & TDF_DETAILS) - { - fprintf (dump_file, "Fixing up noreturn call "); - print_gimple_stmt (dump_file, stmt, 0); - fprintf (dump_file, "\n"); - } - fixup_noreturn_call (stmt); - } - statistics_counter_event (cfun, "Constants propagated", prop_stats.num_const_prop); statistics_counter_event (cfun, "Copies propagated", diff --git a/gcc/tree-ssa-propagate.h b/gcc/tree-ssa-propagate.h index 81b635e0787..9d3a437593e 100644 --- a/gcc/tree-ssa-propagate.h +++ b/gcc/tree-ssa-propagate.h @@ -109,4 +109,41 @@ class substitute_and_fold_engine bool replace_phi_args_in (gphi *); }; +// Class to record statement propagation changes. Upon destruction of +// this class, any possible cleanups to EH are performed. + +class propagate_cleanups +{ +public: + propagate_cleanups (unsigned int *flags = NULL) : m_todo_flags (flags) { } + ~propagate_cleanups (); + void record_change (gimple *old_stmt, gimple *new_stmt, + bool recompute_invariants = true); + void record_eh_change (basic_block); + +protected: + // Basic blocks needing EH fixups. + auto_bitmap m_bbs_fixups; +private: + // Statements that became `noreturn' and need fixups. + auto_vec<gimple *> m_stmts_fixups; + // Set if any cleanups were performed. + bool m_changed; + // If non-NULL and cleanups were performed, this will contain any + // TODO_flags that should be performed after the destructor completes. + unsigned int *m_todo_flags; +}; + +// Note: Use the above class intead of these functions. The only +// reason these are still here are to avoid ripping apart +// tree-ssa-forwprop.c to use the class. Once forwprop is converted, +// these should be removed. +extern void propagate_mark_stmt_for_cleanup + (gimple *old_stmt, gimple *new_stmt, + bitmap bbs_needing_eh_cleanup, + vec<gimple *> *stmts_that_became_noreturn, + bool recompute_invariants = true); +bool propagate_cleanup (bitmap bbs_needing_eh_cleanup, + vec<gimple *> *stmts_that_became_noreturn); + #endif /* _TREE_SSA_PROPAGATE_H */ diff --git a/gcc/tree-ssa-sccvn.c b/gcc/tree-ssa-sccvn.c index a174f18f72a..81d2cc6ebd6 100644 --- a/gcc/tree-ssa-sccvn.c +++ b/gcc/tree-ssa-sccvn.c @@ -70,6 +70,7 @@ along with GCC; see the file COPYING3. If not see #include "tree-scalar-evolution.h" #include "tree-ssa-loop-niter.h" #include "tree-ssa-sccvn.h" +#include "tree-ssa-propagate.h" /* This algorithm is based on the SCC algorithm presented by Keith Cooper and L. Taylor Simpson in "SCC-Based Value numbering" @@ -1870,7 +1871,7 @@ vn_nary_simplify (vn_nary_op_t nary) class eliminate_dom_walker : public dom_walker { public: - eliminate_dom_walker (cdi_direction, bitmap); + eliminate_dom_walker (cdi_direction, bitmap, unsigned *flags); ~eliminate_dom_walker (); virtual edge before_dom_children (basic_block); @@ -1889,18 +1890,17 @@ public: unsigned int eliminations; unsigned int insertions; + /* Post propagation cleanups. */ + propagate_cleanups fixups; + /* SSA names that had their defs inserted by PRE if do_pre. */ bitmap inserted_exprs; - /* Blocks with statements that have had their EH properties changed. */ - bitmap need_eh_cleanup; - /* Blocks with statements that have had their AB properties changed. */ bitmap need_ab_cleanup; /* Local state for the eliminate domwalk. */ auto_vec<gimple *> to_remove; - auto_vec<gimple *> to_fixup; auto_vec<tree> avail; auto_vec<tree> avail_stack; }; @@ -1911,7 +1911,7 @@ class rpo_elim : public eliminate_dom_walker { public: rpo_elim(basic_block entry_) - : eliminate_dom_walker (CDI_DOMINATORS, NULL), entry (entry_) {} + : eliminate_dom_walker (CDI_DOMINATORS, NULL, NULL), entry (entry_) {} ~rpo_elim(); virtual tree eliminate_avail (basic_block, tree op); @@ -4818,18 +4818,18 @@ vn_reference_may_trap (vn_reference_t ref) } eliminate_dom_walker::eliminate_dom_walker (cdi_direction direction, - bitmap inserted_exprs_) + bitmap inserted_exprs_, + unsigned *flags) : dom_walker (direction), do_pre (inserted_exprs_ != NULL), el_todo (0), eliminations (0), insertions (0), + fixups (flags), inserted_exprs (inserted_exprs_) { - need_eh_cleanup = BITMAP_ALLOC (NULL); need_ab_cleanup = BITMAP_ALLOC (NULL); } eliminate_dom_walker::~eliminate_dom_walker () { - BITMAP_FREE (need_eh_cleanup); BITMAP_FREE (need_ab_cleanup); } @@ -5152,8 +5152,7 @@ eliminate_dom_walker::eliminate_stmt (basic_block b, gimple_stmt_iterator *gsi) its EH information. */ if (maybe_clean_or_replace_eh_stmt (orig_stmt, stmt)) { - bitmap_set_bit (need_eh_cleanup, - gimple_bb (stmt)->index); + fixups.record_eh_change (gimple_bb (stmt)); if (dump_file && (dump_flags & TDF_DETAILS)) fprintf (dump_file, " Removed EH side-effects.\n"); } @@ -5237,8 +5236,6 @@ eliminate_dom_walker::eliminate_stmt (basic_block b, gimple_stmt_iterator *gsi) } bool can_make_abnormal_goto = stmt_can_make_abnormal_goto (stmt); - bool was_noreturn = (is_gimple_call (stmt) - && gimple_call_noreturn_p (stmt)); tree vdef = gimple_vdef (stmt); tree vuse = gimple_vuse (stmt); @@ -5391,11 +5388,7 @@ eliminate_dom_walker::eliminate_stmt (basic_block b, gimple_stmt_iterator *gsi) if (modified) { - /* When changing a call into a noreturn call, cfg cleanup - is needed to fix up the noreturn call. */ - if (!was_noreturn - && is_gimple_call (stmt) && gimple_call_noreturn_p (stmt)) - to_fixup.safe_push (stmt); + fixups.record_change (old_stmt, stmt); /* When changing a condition or switch into one we know what edge will be executed, schedule a cfg cleanup. */ if ((gimple_code (stmt) == GIMPLE_COND @@ -5405,16 +5398,8 @@ eliminate_dom_walker::eliminate_stmt (basic_block b, gimple_stmt_iterator *gsi) && TREE_CODE (gimple_switch_index (as_a <gswitch *> (stmt))) == INTEGER_CST)) el_todo |= TODO_cleanup_cfg; - /* If we removed EH side-effects from the statement, clean - its EH information. */ - if (maybe_clean_or_replace_eh_stmt (old_stmt, stmt)) - { - bitmap_set_bit (need_eh_cleanup, - gimple_bb (stmt)->index); - if (dump_file && (dump_flags & TDF_DETAILS)) - fprintf (dump_file, " Removed EH side-effects.\n"); - } - /* Likewise for AB side-effects. */ + /* If we removed AB side-effects from the statement, clean its + AB call edges. */ if (can_make_abnormal_goto && !stmt_can_make_abnormal_goto (stmt)) { @@ -5609,7 +5594,7 @@ eliminate_dom_walker::eliminate_cleanup (bool region_p) stmt = gsi_stmt (gsi); update_stmt (stmt); if (maybe_clean_or_replace_eh_stmt (stmt, stmt)) - bitmap_set_bit (need_eh_cleanup, gimple_bb (stmt)->index); + fixups.record_eh_change (gimple_bb (stmt)); continue; } else @@ -5635,7 +5620,7 @@ eliminate_dom_walker::eliminate_cleanup (bool region_p) basic_block bb = gimple_bb (stmt); unlink_stmt_vdef (stmt); if (gsi_remove (&gsi, true)) - bitmap_set_bit (need_eh_cleanup, bb->index); + fixups.record_eh_change (bb); if (is_gimple_call (stmt) && stmt_can_make_abnormal_goto (stmt)) bitmap_set_bit (need_ab_cleanup, bb->index); if (do_release_defs) @@ -5646,36 +5631,12 @@ eliminate_dom_walker::eliminate_cleanup (bool region_p) el_todo |= TODO_cleanup_cfg; } - /* 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 (!to_fixup.is_empty ()) + if (!bitmap_empty_p (need_ab_cleanup)) { - gimple *stmt = to_fixup.pop (); - - if (dump_file && (dump_flags & TDF_DETAILS)) - { - fprintf (dump_file, "Fixing up noreturn call "); - print_gimple_stmt (dump_file, stmt, 0); - } - - if (fixup_noreturn_call (stmt)) - el_todo |= TODO_cleanup_cfg; + gimple_purge_all_dead_abnormal_call_edges (need_ab_cleanup); + el_todo |= TODO_cleanup_cfg; } - bool do_eh_cleanup = !bitmap_empty_p (need_eh_cleanup); - bool do_ab_cleanup = !bitmap_empty_p (need_ab_cleanup); - - if (do_eh_cleanup) - gimple_purge_all_dead_eh_edges (need_eh_cleanup); - - if (do_ab_cleanup) - gimple_purge_all_dead_abnormal_call_edges (need_ab_cleanup); - - if (do_eh_cleanup || do_ab_cleanup) - el_todo |= TODO_cleanup_cfg; - return el_todo; } @@ -5684,10 +5645,13 @@ eliminate_dom_walker::eliminate_cleanup (bool region_p) unsigned eliminate_with_rpo_vn (bitmap inserted_exprs) { - eliminate_dom_walker walker (CDI_DOMINATORS, inserted_exprs); - - walker.walk (cfun->cfg->x_entry_block_ptr); - return walker.eliminate_cleanup (); + unsigned flags = 0; + { + eliminate_dom_walker walker (CDI_DOMINATORS, inserted_exprs, &flags); + walker.walk (cfun->cfg->x_entry_block_ptr); + flags |= walker.eliminate_cleanup (); + } + return flags; } static unsigned