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 <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
>
