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.
There should be no other changes necessary to DSE for cxa_throw to
be 'pure'.
> >
> > Bootstrapped on x86_64-unknown-linux-gnu, testing in progress.
> >
> > This doesn't seem to be a regression and I'm unsure as to how
> > important it is for Ada (I consider it not important for C/C++),
> > so for now I'll queue it for next stage1.
>
> 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.
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)
