On Thu, Feb 9, 2023 at 12:07 AM Richard Biener via Gcc-patches <gcc-patches@gcc.gnu.org> wrote: > > On Wed, Feb 8, 2023 at 8:14 PM Andrew Pinski via Gcc-patches > <gcc-patches@gcc.gnu.org> wrote: > > > > In simple_dce_from_worklist, we were removing an inline-asm which had a vdef > > (due to clobbering memory) but not unlinking the statement's vdef. > > This fixes that oversight. This was a latent bug exposed recently > > by both VRP and removal of stores to static starting to use > > simple_dce_from_worklist. > > > > OK for trunk (and for GCC 12 after a week)? > > Bootstrapped and tested on x86_64-linux-gnu with no regressions. > > I think this is actually wrong-code - we cannot remove memory side-effects > of a stmt and the > > /* The defining statement needs to be defining only this name. > ASM is the only statement that can define more than one > (non-virtual) name. */ > if (is_a<gasm *>(t) > && !single_ssa_def_operand (t, SSA_OP_DEF)) > continue; > > should use SSA_OP_ALL_DEFS instead.
Yes there is definitely wrong code. Will implement this change and add a testcase which was being definitely being miscompiled (I put the testcase in the bug report already). Thanks, Andrew Pinski > > OK with that change. > > Richard. > > > PR tree-optimization/108684 > > > > gcc/ChangeLog: > > > > * tree-ssa-dce.cc (simple_dce_from_worklist): > > Call unlink_stmt_vdef on the statement before > > removing it. > > > > gcc/testsuite/ChangeLog: > > > > * gcc.c-torture/compile/dce-inline-asm-1.c: New test. > > * gcc.c-torture/compile/dce-inline-asm-2.c: New test. > > > > co-authored-by: Andrew Macleod <amacl...@redhat.com> > > --- > > .../gcc.c-torture/compile/dce-inline-asm-1.c | 15 +++++++++++++++ > > .../gcc.c-torture/compile/dce-inline-asm-2.c | 16 ++++++++++++++++ > > gcc/tree-ssa-dce.cc | 1 + > > 3 files changed, 32 insertions(+) > > create mode 100644 gcc/testsuite/gcc.c-torture/compile/dce-inline-asm-1.c > > create mode 100644 gcc/testsuite/gcc.c-torture/compile/dce-inline-asm-2.c > > > > diff --git a/gcc/testsuite/gcc.c-torture/compile/dce-inline-asm-1.c > > b/gcc/testsuite/gcc.c-torture/compile/dce-inline-asm-1.c > > new file mode 100644 > > index 00000000000..a9f02e44bd7 > > --- /dev/null > > +++ b/gcc/testsuite/gcc.c-torture/compile/dce-inline-asm-1.c > > @@ -0,0 +1,15 @@ > > +/* PR tree-optimization/108684 */ > > +/* This used to ICE as when we remove the store to > > + `t`, we also would remove the inline-asm which > > + had a VDEF on it but we didn't update the > > + VUSE that was later on. */ > > +static int t; > > + > > +int f (int *a) > > +{ > > + int t1; > > + asm (" " : "=X" (t1) : : "memory"); > > + t = t1; > > + return *a; > > +} > > + > > diff --git a/gcc/testsuite/gcc.c-torture/compile/dce-inline-asm-2.c > > b/gcc/testsuite/gcc.c-torture/compile/dce-inline-asm-2.c > > new file mode 100644 > > index 00000000000..a41b16e4bd0 > > --- /dev/null > > +++ b/gcc/testsuite/gcc.c-torture/compile/dce-inline-asm-2.c > > @@ -0,0 +1,16 @@ > > +/* PR tree-optimization/108684 */ > > +/* This used to ICE as when we removed the > > + __builtin_unreachable in VRP, as we > > + would also remove the branch and the > > + inline-asm. The inline-asm had a VDEF on it, > > + which we didn't update further along and > > + not have the VDEF on the return statement > > + updated. */ > > + > > +int f (int a) > > +{ > > + asm (" " : "=X" (a) : : "memory"); > > + if (a) > > + return 0; > > + __builtin_unreachable(); > > +} > > diff --git a/gcc/tree-ssa-dce.cc b/gcc/tree-ssa-dce.cc > > index b2fe9f4f55e..752785541e4 100644 > > --- a/gcc/tree-ssa-dce.cc > > +++ b/gcc/tree-ssa-dce.cc > > @@ -2140,6 +2140,7 @@ simple_dce_from_worklist (bitmap worklist) > > remove_phi_node (&gsi, true); > > else > > { > > + unlink_stmt_vdef (t); > > gsi_remove (&gsi, true); > > release_defs (t); > > } > > -- > > 2.31.1 > >