Message ID | 20230208191348.1580462-1-apinski@marvell.com |
---|---|
State | New |
Headers | show |
Series | tree-optimization: [PR108684] ICE in verify_ssa due to simple_dce_from_worklist | expand |
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. 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 <amacleod@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 >
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 <amacleod@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 > >
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); }