| Message ID | 20220128102939.4C0CF13D17@imap2.suse-dmz.suse.de |
|---|---|
| State | New |
| Headers | show |
| Series | tree-optimization/104263 - avoid retaining abnormal edges for non-call/goto stmts | expand |
On Fri, Jan 28, 2022 at 11:29:38AM +0100, Richard Biener wrote: > This removes a premature optimization from > gimple_purge_dead_abnormal_call_edges which, after eliding the > last setjmp (or computed goto) statement from a function and > thus clearing cfun->calls_setjmp, leaves us with the abnormal > edges from other calls that are elided for example via inlining > or DCE. That's a CFG / IL combination that should be impossible > (not addressing the fact that with cfun->calls_setjmp and > cfun->has_nonlocal_label cleared we should not have any abnormal > edge at all). > > For the testcase in the PR this means that IPA inlining will > remove the abormal edges from the block after inlining the call > the edge was coming from. Couldn't DCE when it clears calls_setjmp and doesn't set it again (I think we never clear has_nonlocal_label) temporarily set calls_setjmp and gimple_purge_all_dead_abnormal_call_edges with it? Or have next to calls_setjmp a maybe_calls_setjmp flag that would be sticky like has_nonlocal_labels and would be never cleared? Jakub
On Fri, 28 Jan 2022, Jakub Jelinek wrote: > On Fri, Jan 28, 2022 at 11:29:38AM +0100, Richard Biener wrote: > > This removes a premature optimization from > > gimple_purge_dead_abnormal_call_edges which, after eliding the > > last setjmp (or computed goto) statement from a function and > > thus clearing cfun->calls_setjmp, leaves us with the abnormal > > edges from other calls that are elided for example via inlining > > or DCE. That's a CFG / IL combination that should be impossible > > (not addressing the fact that with cfun->calls_setjmp and > > cfun->has_nonlocal_label cleared we should not have any abnormal > > edge at all). > > > > For the testcase in the PR this means that IPA inlining will > > remove the abormal edges from the block after inlining the call > > the edge was coming from. > > Couldn't DCE when it clears calls_setjmp and doesn't set it again > (I think we never clear has_nonlocal_label) temporarily set > calls_setjmp and gimple_purge_all_dead_abnormal_call_edges > with it? > Or have next to calls_setjmp a maybe_calls_setjmp flag that > would be sticky like has_nonlocal_labels and would be never cleared? I suppose we could do things like this. Note in CFG cleanup we call gimple_purge_dead_eh_edges on each block but not gimple_purge_dead_abnormal_call_edges. DCE, when resetting the flag could also manually axe all abnormal edges in the function. Still I think assuming there are no abnormal edges when neither of the flag is set is premature (as can be seen here). I also don't think what we do in the function is very timing critical, but sure, we walk all successor edges. uninit has interesting code checking ->calls_setjmp conditionally ignoring abnormal SSA names but only then ... (it should be able to ignore them when _none_ of the flags is set instead). That said, gimple_purge_dead_abnormal_call_edges wants to check "are there possibly any abnormal edges in the function" and clearly testing just the flags doesn't do it but resetting the flag was important enough to cut out (sometimes bogus?) checks before optimizations. Richard.
On Fri, Jan 28, 2022 at 12:04:00PM +0100, Richard Biener wrote: > Still I think assuming there are no abnormal edges when neither > of the flag is set is premature (as can be seen here). I also > don't think what we do in the function is very timing critical, > but sure, we walk all successor edges. Ok then. Jakub
On Fri, 28 Jan 2022, Jakub Jelinek wrote: > On Fri, Jan 28, 2022 at 12:04:00PM +0100, Richard Biener wrote: > > Still I think assuming there are no abnormal edges when neither > > of the flag is set is premature (as can be seen here). I also > > don't think what we do in the function is very timing critical, > > but sure, we walk all successor edges. > > Ok then. Just to add - gimple_purge_dead_abnormal_call_edges should only be called if the caller determined a possible change. I've checked and only fixup_cfg calls it unconditionally (I guess on purpose). Richard.
diff --git a/gcc/testsuite/gcc.dg/tree-ssa/inline-13.c b/gcc/testsuite/gcc.dg/tree-ssa/inline-13.c new file mode 100644 index 00000000000..94d8a9c709e --- /dev/null +++ b/gcc/testsuite/gcc.dg/tree-ssa/inline-13.c @@ -0,0 +1,27 @@ +/* { dg-do compile } */ +/* { dg-options "-O2 -fno-early-inlining -fdump-tree-fixup_cfg3" } */ + +int n; + +static int +bar (void) +{ + int a; + + n = 0; + a = 0; + + return n; +} + +__attribute__ ((pure, returns_twice)) int +foo (void) +{ + n = bar () + 1; + foo (); + + return 0; +} + +/* Abnormal edges should be properly elided after IPA inlining of bar. */ +/* { dg-final { scan-tree-dump-times "bb" 1 "fixup_cfg3" } } */ diff --git a/gcc/tree-cfg.cc b/gcc/tree-cfg.cc index 2340cd7cef0..260a7fb97c6 100644 --- a/gcc/tree-cfg.cc +++ b/gcc/tree-cfg.cc @@ -8916,10 +8916,6 @@ gimple_purge_dead_abnormal_call_edges (basic_block bb) edge_iterator ei; gimple *stmt = last_stmt (bb); - if (!cfun->has_nonlocal_label - && !cfun->calls_setjmp) - return false; - if (stmt && stmt_can_make_abnormal_goto (stmt)) return false;