On Tue, 17 Jan 2023, Jan Hubicka wrote:
> > > 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.
> >
> > I suppose i_read_no_global_memory is const here. Not sure why that
> Suppose we have:
>
> void
> i_read_no_global_memory ()
> {
> throw(0);
> }
>
> If cxa_throw itself was annotated as 'p' rahter than 'c' ipa-modref will
> believe that cxa_throw will read any global memory and will propagate it
> to all callers. So fn() will be also marked as reading all global
> memory.
Sure - but for the purpose of local optimizations in
i_read_no_global_memory cxa_throw has to appear to read memory.
Having a VUSE there dependent on whether the function performs any
load or store would be quite ugly. Instead modref could special-case
cxa_throw and not treat it as reading memory (like it already does
for the return stmt I suppose - that also has a VUSE).
> > should make it pure? Only if anything throws externally (not catched
> > in fn) should force it to be pure, no?
> >
> > Of course for IPA purposes whether 'fn' is to be considered const
> > or pure depends on whether its exceptions are catched in the context
> > where that's interesting - that is, whether the EH side-effect is
> > explicitely or implicitely modeled.
>
> We have two things here. const/pure attributes 'c'/'p' fnspec
> specifiers. const/pure implies that function call can be removed when
> result is not necessary. This is not the case of funcitons calling
> throw() (we have -fdelete-dead-exceptions for noncall exceptions and
> those would be OK). However 'c'/'p' is about memory side effects only
> and it is safe for i_read_no_global_memory.
>
> With the C++ FE change adding fnspec to EH handling modref will detect
> both i_read_no_global_memory and fn() as 'c'. It won't infer const
> attribute that is something I can implement later.
> We are very poor on detecting scenarios where all exceptions thrown are
> actually caught. It is long time on my TODO to fix that, so probably
> next stage1 is time to look into that.
>
> >
> > > 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.
> >
> > Yes, I think all control transfer instructions need a VUSE.
>
> I think it is right way to go. So operands_scanner::parse_ssa_operands
> can add vuse to anything that can_throw_external_p (like it does for
> GIMPLE_RETURN) and passes like DSE can test for it and understand that
> on the EH path the globally accessible memory is live and thus "used" by
> the statement.
>
> I can try to cook up a patch.
The problem is IIRC GIMPLE_RESX which doesn't derive from
gimple_statement_with_memory_ops_base. There's a bugzilla I can't find
right now refering to this issue.
Richard.
> Thanks,
> Honza
> >
> > Richard.
> >
> > > > > 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)
> > >
> >
> > --
> > 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)
>
--
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)
