Message ID | 20230210014311.1626049-1-apinski@marvell.com |
---|---|
State | New |
Headers | show |
Series | [COMMITTED] tree-optimization: [PR108684] ICE in verify_ssa due to simple_dce_from_worklist | expand |
On Fri, Feb 10, 2023 at 2:47 AM 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. > We should not be removing inline-asm which have a vdef as this code > does not check to the store. > 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. > > Committed as approved. > Bootstrapped and tested on x86_64-linux-gnu with no regressions. > > PR tree-optimization/108684 > > gcc/ChangeLog: > > * tree-ssa-dce.cc (simple_dce_from_worklist): > Check all ssa names and not just non-vdef ones > before accepting the inline-asm. > 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. > * gcc.dg/tree-ssa/pr108684-1.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/testsuite/gcc.dg/tree-ssa/pr108684-1.c | 18 ++++++++++++++++++ > gcc/tree-ssa-dce.cc | 5 +++-- > 4 files changed, 52 insertions(+), 2 deletions(-) > 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 > create mode 100644 gcc/testsuite/gcc.dg/tree-ssa/pr108684-1.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/testsuite/gcc.dg/tree-ssa/pr108684-1.c b/gcc/testsuite/gcc.dg/tree-ssa/pr108684-1.c > new file mode 100644 > index 00000000000..3ba206f765e > --- /dev/null > +++ b/gcc/testsuite/gcc.dg/tree-ssa/pr108684-1.c > @@ -0,0 +1,18 @@ > +/* { dg-do compile } */ > +/* { dg-options "-O2 -fdump-tree-optimized" } */ > + > + > +static int t; > + > +int f (int *a) > +{ > + int t1, t2 = 0; > + asm ("shouldshowupstill %1" : "=r" (t1), "=m"(t2) : : ); > + t = t1; > + return t2; > +} > + > +/* Check to make sure DCE does not remove the inline-asm as it writes to t2. */ > +/* We used to DCE this inline-asm when removing the store to t. */ > +/* { dg-final { scan-assembler "shouldshowupstill" } } */ > +/* { dg-final { scan-tree-dump-times "shouldshowupstill" 1 "optimized" } } */ > diff --git a/gcc/tree-ssa-dce.cc b/gcc/tree-ssa-dce.cc > index b2fe9f4f55e..1f1ec2c8710 100644 > --- a/gcc/tree-ssa-dce.cc > +++ b/gcc/tree-ssa-dce.cc > @@ -2108,9 +2108,9 @@ simple_dce_from_worklist (bitmap worklist) > > /* The defining statement needs to be defining only this name. > ASM is the only statement that can define more than one > - (non-virtual) name. */ > + name. */ > if (is_a<gasm *>(t) > - && !single_ssa_def_operand (t, SSA_OP_DEF)) > + && !single_ssa_def_operand (t, SSA_OP_ALL_DEFS)) > continue; > > /* Don't remove statements that are needed for non-call > @@ -2140,6 +2140,7 @@ simple_dce_from_worklist (bitmap worklist) > remove_phi_node (&gsi, true); > else > { > + unlink_stmt_vdef (t); note this should never do anything > gsi_remove (&gsi, true); > release_defs (t); > } > -- > 2.27.0 >
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/testsuite/gcc.dg/tree-ssa/pr108684-1.c b/gcc/testsuite/gcc.dg/tree-ssa/pr108684-1.c new file mode 100644 index 00000000000..3ba206f765e --- /dev/null +++ b/gcc/testsuite/gcc.dg/tree-ssa/pr108684-1.c @@ -0,0 +1,18 @@ +/* { dg-do compile } */ +/* { dg-options "-O2 -fdump-tree-optimized" } */ + + +static int t; + +int f (int *a) +{ + int t1, t2 = 0; + asm ("shouldshowupstill %1" : "=r" (t1), "=m"(t2) : : ); + t = t1; + return t2; +} + +/* Check to make sure DCE does not remove the inline-asm as it writes to t2. */ +/* We used to DCE this inline-asm when removing the store to t. */ +/* { dg-final { scan-assembler "shouldshowupstill" } } */ +/* { dg-final { scan-tree-dump-times "shouldshowupstill" 1 "optimized" } } */ diff --git a/gcc/tree-ssa-dce.cc b/gcc/tree-ssa-dce.cc index b2fe9f4f55e..1f1ec2c8710 100644 --- a/gcc/tree-ssa-dce.cc +++ b/gcc/tree-ssa-dce.cc @@ -2108,9 +2108,9 @@ simple_dce_from_worklist (bitmap worklist) /* The defining statement needs to be defining only this name. ASM is the only statement that can define more than one - (non-virtual) name. */ + name. */ if (is_a<gasm *>(t) - && !single_ssa_def_operand (t, SSA_OP_DEF)) + && !single_ssa_def_operand (t, SSA_OP_ALL_DEFS)) continue; /* Don't remove statements that are needed for non-call @@ -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); }