On Tue, 28 Jan 2025, Martin Jambor wrote:
> Hi,
>
> On Tue, Jan 28 2025, Richard Biener wrote:
> > On Mon, 27 Jan 2025, Martin Jambor wrote:
> >
> >> Hi,
> >>
> >> Zhendong Su and Michal Jireš found out that our gimple DSE pass can,
> >> under fairly specific conditions, remove a noreturn call which then
> >> leaves behind a "normal" BB with no successor edges which following
> >> passes do not expect. This patch simply tells the pass to leave such
> >> calls alone even when they otherwise appear to be dead.
> >>
> >> Interestingly, our CFG verifier does not report this. I'll put on my
> >> todo list to add a test for it in the next stage 1.
> >>
> >> Bootstrapped and tested on x86_64-linux, OK for master?
> >>
> >> Thanks,
> >>
> >> Martin
> >>
> >>
> >> gcc/ChangeLog:
> >>
> >> 2025-01-27 Martin Jambor <mjam...@suse.cz>
> >>
> >> PR tree-optimization/117892
> >> * tree-ssa-dse.cc (dse_optimize_call): Leave noreturn calls alone.
> >>
> >> gcc/testsuite/ChangeLog:
> >>
> >> 2025-01-27 Martin Jambor <mjam...@suse.cz>
> >>
> >> PR tree-optimization/117892
> >> * gcc.dg/tree-ssa/pr117892.c: New test.
> >> * gcc.dg/tree-ssa/pr118517.c: Likewise.
> >>
> >> co-authored-by: Michal Jireš <mji...@suse.cz>
> >> ---
> >> gcc/testsuite/gcc.dg/tree-ssa/pr117892.c | 17 +++++++++++++++++
> >> gcc/testsuite/gcc.dg/tree-ssa/pr118517.c | 11 +++++++++++
> >> gcc/tree-ssa-dse.cc | 5 +++--
> >> 3 files changed, 31 insertions(+), 2 deletions(-)
> >> create mode 100644 gcc/testsuite/gcc.dg/tree-ssa/pr117892.c
> >> create mode 100644 gcc/testsuite/gcc.dg/tree-ssa/pr118517.c
> >>
> >> diff --git a/gcc/testsuite/gcc.dg/tree-ssa/pr117892.c
> >> b/gcc/testsuite/gcc.dg/tree-ssa/pr117892.c
> >> new file mode 100644
> >> index 00000000000..d9b9c15095f
> >> --- /dev/null
> >> +++ b/gcc/testsuite/gcc.dg/tree-ssa/pr117892.c
> >> @@ -0,0 +1,17 @@
> >> +/* { dg-do compile } */
> >> +/* { dg-options "-O1" } */
> >> +
> >> +
> >> +volatile int a;
> >> +void b(int *c) {
> >> + int *d = 0;
> >> + *c = 0;
> >> + *d = 0;
> >> + __builtin_abort();
> >> +}
> >> +int main() {
> >> + int f;
> >> + if (a)
> >> + b(&f);
> >> + return 0;
> >> +}
> >> diff --git a/gcc/testsuite/gcc.dg/tree-ssa/pr118517.c
> >> b/gcc/testsuite/gcc.dg/tree-ssa/pr118517.c
> >> new file mode 100644
> >> index 00000000000..3a34f6788a9
> >> --- /dev/null
> >> +++ b/gcc/testsuite/gcc.dg/tree-ssa/pr118517.c
> >> @@ -0,0 +1,11 @@
> >> +/* { dg-do compile } */
> >> +/* { dg-options "-O1 -fno-ipa-pure-const" } */
> >> +
> >> +void __attribute__((noreturn)) bar(void) {
> >> + __builtin_unreachable ();
> >> +}
> >> +
> >> +int p;
> >> +void foo() {
> >> + if (p) bar();
> >> +}
> >> diff --git a/gcc/tree-ssa-dse.cc b/gcc/tree-ssa-dse.cc
> >> index 753d7ef148b..492080a7eb0 100644
> >> --- a/gcc/tree-ssa-dse.cc
> >> +++ b/gcc/tree-ssa-dse.cc
> >> @@ -1396,8 +1396,9 @@ dse_optimize_call (gimple_stmt_iterator *gsi,
> >> sbitmap live_bytes)
> >> if (!node)
> >> return false;
> >>
> >> - if (stmt_could_throw_p (cfun, stmt)
> >> - && !cfun->can_delete_dead_exceptions)
> >> + if ((stmt_could_throw_p (cfun, stmt)
> >> + && !cfun->can_delete_dead_exceptions)
> >> + || gimple_call_noreturn_p (stmt))
> >
> > Please use the same test as is used in tree-ssa-dce.cc:
> >
> > /* Never elide a noreturn call we pruned control-flow for. */
> > if ((gimple_call_flags (call) & ECF_NORETURN)
> > && gimple_call_ctrl_altering_p (call))
> > {
> > mark_stmt_necessary (call, true);
> > return;
> > }
>
> OK, but just to make sure I understand: You do want me to leave the
> !cfun->can_delete_dead_exceptions check there, right, so the condition
> would become:
>
> if ((stmt_could_throw_p (cfun, stmt)
> && !cfun->can_delete_dead_exceptions)
> || (gimple_call_flags (call) & ECF_NORETURN)
> && gimple_call_ctrl_altering_p (call))
>
>
> Right?
Yes, of course.
> On x86_64 at least we do not have a testcase that would exercise the
> can_delete_dead_exceptions part of the condition - not even an Ada
> testcase - so it is not easy to think about it for me.
>
> And I also cannot stop wondering: Is explicitely checking for the
> ECF_NORETURN bit really better than gimple_call_noreturn_p which does
> exactly the same thing?
No, my point was the ..
> And perhaps more importantly, are there noreturn calls which are not
> gimple_call_ctrl_altering_p? What are they?
.. gimple_call_ctrl_altering_p flag check. Only ECF_NORETURN calls
that have this set end a basic-block. Others we can freely delete
without ICEing.
That said, we should try to reduce subtle differences in "same" code,
thus my comment.
Richard.
> Thanks,
>
> Martin
>
--
Richard Biener <rguent...@suse.de>
SUSE Software Solutions Germany GmbH,
Frankenstrasse 146, 90461 Nuernberg, Germany;
GF: Ivo Totev, Andrew McDonald, Werner Knoblich; (HRB 36809, AG Nuernberg)
