> On Tue, 17 Jan 2023, Jan Hubicka wrote:
>
> > > The following fixes a long-standing bug with DSE removing stores as
> > > dead even though they are live across non-call exceptional flow.
> > > This affects both GIMPLE and RTL DSE and the fix is similar in
> > > making externally throwing statements uses of non-local stores.
> > > Note this doesn't fix the GIMPLE side when the throwing statement
> > > does not involve a load or a store because then the statement does
> > > not have virtual operands and thus is not visited by GIMPLE DSE.
> >
> > Thanks for looking into this.
> > My main motivation for poking on this is the patch to add fnspec to
> > throw/catch machinery
> > https://gcc.gnu.org/pipermail/gcc-patches/2022-June/597124.html
> >
> > The eh6.C testcase is misoptimized by current trun with the patch.
> > I think I can adjust it for the throwing function to have no vops and it
> > will still get misoptimized by DSE.
>
> I think conceptually the "throw" may not be 'const' - it has to be
> 'pure' at most since the program point the control is transfered to
> can inspect memory.
We don't use same argumentation about other control flow statements.
The following:
fn()
{
try {
i_read_no_global_memory ();
} catch (...)
{
reutrn 1;
}
return 0;
}
should be detected as const. Marking throw pure would make fn pure too.
With noncall exceptions a=b/c also can transfer to place that inspect
memory. We may want all statements with can_throw_extenral to have VUSE
on them (like with return) since they may cause function to return, but
I think fnspec is wrong place to model this.
> > According to compiler explorer testcase:
> > struct a{int a,b,c,d,e;};
> > void
> > test(struct a * __restrict a, struct a *b)
> > {
> > *a = (struct a){0,1,2,3,4};
> > *a = *b;
> > }
> > Is compiled correctly by GCC 5.4 and first miscopmiled by 6.1, so I
> > think it is a regression. (For C++ not very important one as
> > -fnon-call-exceptions is not very common for C++)
>
> Ah, yes - RTL DSE probably is too weak for this and GIMPLE DSE
> didn't handle aggregates well at some point.
Yep, we never handled it really correctly but were weaker on optimizing
and thus also producing wrong code :)
Honza
>
> Richard.
>
> >
> > Honza
> > >
> > > PR middle-end/106075
> > > * dse.cc (scan_insn): Consider externally throwing insns
> > > to read from not frame based memory.
> > > * tree-ssa-dse.cc (dse_classify_store): Consider externally
> > > throwing uses to read from global memory.
> > >
> > > * gcc.dg/torture/pr106075-1.c: New testcase.
> > > ---
> > > gcc/dse.cc | 5 ++++
> > > gcc/testsuite/gcc.dg/torture/pr106075-1.c | 36 +++++++++++++++++++++++
> > > gcc/tree-ssa-dse.cc | 8 ++++-
> > > 3 files changed, 48 insertions(+), 1 deletion(-)
> > > create mode 100644 gcc/testsuite/gcc.dg/torture/pr106075-1.c
> > >
> > > diff --git a/gcc/dse.cc b/gcc/dse.cc
> > > index a2db8d1cc32..7e258b81f66 100644
> > > --- a/gcc/dse.cc
> > > +++ b/gcc/dse.cc
> > > @@ -2633,6 +2633,11 @@ scan_insn (bb_info_t bb_info, rtx_insn *insn, int
> > > max_active_local_stores)
> > > return;
> > > }
> > >
> > > + /* An externally throwing statement may read any memory that is not
> > > + relative to the frame. */
> > > + if (can_throw_external (insn))
> > > + add_non_frame_wild_read (bb_info);
> > > +
> > > /* Assuming that there are sets in these insns, we cannot delete
> > > them. */
> > > if ((GET_CODE (PATTERN (insn)) == CLOBBER)
> > > diff --git a/gcc/testsuite/gcc.dg/torture/pr106075-1.c
> > > b/gcc/testsuite/gcc.dg/torture/pr106075-1.c
> > > new file mode 100644
> > > index 00000000000..b9affbf1082
> > > --- /dev/null
> > > +++ b/gcc/testsuite/gcc.dg/torture/pr106075-1.c
> > > @@ -0,0 +1,36 @@
> > > +/* { dg-do run { target *-*-linux* } } */
> > > +/* { dg-additional-options "-fnon-call-exceptions" } */
> > > +
> > > +#include <unistd.h>
> > > +#include <signal.h>
> > > +#include <stdlib.h>
> > > +
> > > +int a = 1;
> > > +short *b;
> > > +void __attribute__((noipa))
> > > +test()
> > > +{
> > > + a=12345;
> > > + *b=0;
> > > + a=1;
> > > +}
> > > +
> > > +void check (int i)
> > > +{
> > > + if (a != 12345)
> > > + abort ();
> > > + exit (0);
> > > +}
> > > +
> > > +int
> > > +main ()
> > > +{
> > > + struct sigaction s;
> > > + sigemptyset (&s.sa_mask);
> > > + s.sa_handler = check;
> > > + s.sa_flags = 0;
> > > + sigaction (SIGSEGV, &s, NULL);
> > > + test();
> > > + abort ();
> > > + return 0;
> > > +}
> > > diff --git a/gcc/tree-ssa-dse.cc b/gcc/tree-ssa-dse.cc
> > > index 46ab57d5754..b2e2359c3da 100644
> > > --- a/gcc/tree-ssa-dse.cc
> > > +++ b/gcc/tree-ssa-dse.cc
> > > @@ -960,6 +960,7 @@ dse_classify_store (ao_ref *ref, gimple *stmt,
> > > auto_bitmap visited;
> > > std::unique_ptr<data_reference, void(*)(data_reference_p)>
> > > dra (nullptr, free_data_ref);
> > > + bool maybe_global = ref_may_alias_global_p (ref, false);
> > >
> > > if (by_clobber_p)
> > > *by_clobber_p = true;
> > > @@ -1038,6 +1039,11 @@ dse_classify_store (ao_ref *ref, gimple *stmt,
> > > last_phi_def = as_a <gphi *> (use_stmt);
> > > }
> > > }
> > > + /* If the stmt can throw externally and the store is
> > > + visible in the context unwound to the store is live. */
> > > + else if (maybe_global
> > > + && stmt_can_throw_external (cfun, use_stmt))
> > > + return DSE_STORE_LIVE;
> > > /* If the statement is a use the store is not dead. */
> > > else if (ref_maybe_used_by_stmt_p (use_stmt, ref))
> > > {
> > > @@ -1116,7 +1122,7 @@ dse_classify_store (ao_ref *ref, gimple *stmt,
> > > just pretend the stmt makes itself dead. Otherwise fail. */
> > > if (defs.is_empty ())
> > > {
> > > - if (ref_may_alias_global_p (ref, false))
> > > + if (maybe_global)
> > > return DSE_STORE_LIVE;
> > >
> > > if (by_clobber_p)
> > > --
> > > 2.35.3
> >
>
> --
> Richard Biener <rguent...@suse.de>
> SUSE Software Solutions Germany GmbH, Frankenstrasse 146, 90461 Nuernberg,
> Germany; GF: Ivo Totev, Andrew Myers, Andrew McDonald, Boudien Moerman;
> HRB 36809 (AG Nuernberg)
