From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-lf1-x130.google.com (mail-lf1-x130.google.com [IPv6:2a00:1450:4864:20::130]) by sourceware.org (Postfix) with ESMTPS id D95FB3858C3A for ; Fri, 10 Feb 2023 07:46:22 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org D95FB3858C3A Authentication-Results: sourceware.org; dmarc=pass (p=none dis=none) header.from=gmail.com Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=gmail.com Received: by mail-lf1-x130.google.com with SMTP id f34so6984433lfv.10 for ; Thu, 09 Feb 2023 23:46:22 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20210112; h=cc:to:subject:message-id:date:from:in-reply-to:references :mime-version:from:to:cc:subject:date:message-id:reply-to; bh=pWZX8jQjzW8gdCVkZQrTtA87Qe8DuXbTSJzi70EKIg8=; b=Be6ndtjZwTdIESsVLrpCHNGiP6msBmF0kfPuNnFdrBlGNrR1Mxc6pNt82M7nPiz5NM gwHK8+2pbzohatp/Hd/5RzE5CUodJY6tRC9Uim0836UhkCvuA3o8K2ETCTDzoL0N0iwS JMuMmBhWF67CHUaE58LID9+9lyZOnZHFJ7ZrywJymcum4T3hZ+Uq8ft63L7hyro9pCEh nRfmOUXMWRFN+0bvftUap56CJOkcYsg1By7U/4YL4GIyYyZcYDPmaw4Iy35/ZaxlvohX 8kIe/pbqMPR4bzt4mBkd3ydQfmZ6E0gQEL5rCnSUbcT7T27fo4inbLt8bzENu42yQYbX QBJQ== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=cc:to:subject:message-id:date:from:in-reply-to:references :mime-version:x-gm-message-state:from:to:cc:subject:date:message-id :reply-to; bh=pWZX8jQjzW8gdCVkZQrTtA87Qe8DuXbTSJzi70EKIg8=; b=PiMYYTqc5150CnZu41uKhuSlUyXrspRhxjmCLHfgMvZFE6sTIHWqxxn+mR3zQj6jay eOqT+sd2dTQO03eDovrXGRQ/zk8+Vf38fj3eSAAB1QtppkMWP4yFWEv+BtTCvsDCUSUe JBNC0Wq8qiaCc1V+uVs0vyricBSgnqaphgjgexpFULhGdd+ujUk695bRXzajyH4kWNKA rsLwZUA3ylQp75NwsEhMgI3VgGJ0Paeyo6jKRYiD//dUmfgWCdVjVyt2Hw0tpVV6fTjm xRV+D4N/xc69MVDHZLnvmDVMwKJh+dzjP4k2MsQM735foFYHrRoVwZmHdKqXqJAfz7Fn EZwQ== X-Gm-Message-State: AO0yUKV705hvlxB2uUSMdE93yVdGkDuLoA+OdexgWG2yB5TjA1n28+P3 V/b8AJFogz8+llG299J6BMV5KMoRd0nXFK+GCIk= X-Google-Smtp-Source: AK7set89hTtk4KPEwrUQlUiKq1TqMi73vjOJznWEc2fu0CP4KlzUZLzFTKZiLP5VKz2deiK7Wd9x6I5KxxkJGmd4iPU= X-Received: by 2002:a05:6512:491:b0:4b6:ee33:d441 with SMTP id v17-20020a056512049100b004b6ee33d441mr2497399lfq.3.1676015181459; Thu, 09 Feb 2023 23:46:21 -0800 (PST) MIME-Version: 1.0 References: <20230210014311.1626049-1-apinski@marvell.com> In-Reply-To: <20230210014311.1626049-1-apinski@marvell.com> From: Richard Biener Date: Fri, 10 Feb 2023 08:46:09 +0100 Message-ID: Subject: Re: [COMMITTED] tree-optimization: [PR108684] ICE in verify_ssa due to simple_dce_from_worklist To: Andrew Pinski Cc: gcc-patches@gcc.gnu.org, Andrew Macleod Content-Type: text/plain; charset="UTF-8" X-Spam-Status: No, score=-7.1 required=5.0 tests=BAYES_00,DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,FREEMAIL_FROM,GIT_PATCH_0,RCVD_IN_DNSWL_NONE,SPF_HELO_NONE,SPF_PASS,TXREP autolearn=ham autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on server2.sourceware.org List-Id: On Fri, Feb 10, 2023 at 2:47 AM Andrew Pinski via Gcc-patches 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 > --- > .../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(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 >