From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-lj1-x22a.google.com (mail-lj1-x22a.google.com [IPv6:2a00:1450:4864:20::22a]) by sourceware.org (Postfix) with ESMTPS id 59FD53857C40 for ; Wed, 19 Jul 2023 08:34:02 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org 59FD53857C40 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-lj1-x22a.google.com with SMTP id 38308e7fff4ca-2b74310566cso104045201fa.2 for ; Wed, 19 Jul 2023 01:34:02 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20221208; t=1689755641; x=1692347641; h=content-transfer-encoding:cc:to:subject:message-id:date:from :in-reply-to:references:mime-version:from:to:cc:subject:date :message-id:reply-to; bh=cohsFa4/iDT8IF2vJRy1uapd6eJovZSsEvz7KozpCcY=; b=BfWVsxtKJBZAGYmm6XEqydfSaI2VQFDjJzCtkmRHpXdE/YTXvCAPIEJMaZML8gVwNW FhlI0QQyZuvIQI1Lu3yyK+XwkIdghabi2L54Nkl+FU68occLxn4riCO7pS/lRtx9CNor AtXI93e0UahK0+huVauiSX+V/d/g8hRf08r3N8vzVAv0YQoIyImUxTnDNPndV4fhoheE QvH/6wNprFyuViXRiKG9rA7FbWmx8/NGqZkbVlak0GEQ3K0h4kiPquN0F5JtuuK6Xi75 Dw8hRIGWwygEIS0gAV1UC3fSHRApgwhoJey/a7/oPTFWXUTFPgcrn8iCpBHuVzEC2Ugj gJww== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20221208; t=1689755641; x=1692347641; h=content-transfer-encoding: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=cohsFa4/iDT8IF2vJRy1uapd6eJovZSsEvz7KozpCcY=; b=egqFMhnNzbjGU6x7v4AfZnNIk0OZ1IZ7jTpV6x7ovidMPr6zWM5aNWn8X6IjbX42Rq OBYswt6kxsFzNBor4VgX0qVfCjrCGF3dmB+8Ayi3F6iIy8pI0z7q7nZ4XLwtxp3IaVy9 ENNRe1lsp6Z36n6VgyHUk71oDcuxBezdG1O6tMNnWfE5pNjxThOhUWc/fcQxrOrafgq+ j/7pDAPaYGgwdISS7inGcVzRL8QK7qSH0E/o0uRd50OOyi9s3EeiVM79JDwnWGg7VI4F RfKnvAu79LqMzo7hsRHBu+xPdAes5gMPZM/kFHXuxuW6Vh5jnwOV6jVeYFrquQDSiUaU 0Aig== X-Gm-Message-State: ABy/qLb2RA3VsprDR4cEv64E+NNspkNaBpvAsOfWod+aZu3AYdmem6LT 9dzmV5IPqYAE1an74Gf2p/1WrifDHNNVCRiPgiA= X-Google-Smtp-Source: APBJJlFnZdL2RRjHZZds+xKISv58nYXwto7n6+/qIH7Lt7AEunbE/1UzX4gb8UkwYaWHJ2xPO8CR1rnFSNB9r25niUw= X-Received: by 2002:a2e:b164:0:b0:2b6:364:c153 with SMTP id a4-20020a2eb164000000b002b60364c153mr12800118ljm.14.1689755640554; Wed, 19 Jul 2023 01:34:00 -0700 (PDT) MIME-Version: 1.0 References: <20230715031957.1147225-1-apinski@marvell.com> <20230715031957.1147225-2-apinski@marvell.com> In-Reply-To: <20230715031957.1147225-2-apinski@marvell.com> From: Richard Biener Date: Wed, 19 Jul 2023 10:33:27 +0200 Message-ID: Subject: Re: [PATCH 2/2] [PATCH] Fix tree-opt/110252: wrong code due to phiopt using flow sensitive info during match To: Andrew Pinski Cc: gcc-patches@gcc.gnu.org Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable X-Spam-Status: No, score=-7.5 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,T_SCC_BODY_TEXT_LINE 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 Sat, Jul 15, 2023 at 5:21=E2=80=AFAM Andrew Pinski via Gcc-patches wrote: > > Match will query ranger via tree_nonzero_bits/get_nonzero_bits for 2 and = 3rd > operand of the COND_EXPR and phiopt tries to do create the COND_EXPR even= if we moving > one statement. That one statement could have some flow sensitive informat= ion on it > based on the condition that is for the COND_EXPR but that might create wr= ong code > if the statement was moved out. > > This is similar to the previous version of the patch except now we use > flow_sensitive_info_storage instead of manually doing the save/restore > and also handle all defs on a gimple statement rather than just for lhs > of the gimple statement. Oh and a few more testcases were added that > was failing before. > > OK? Bootsrapped and tested on x86_64-linux-gnu with no regressions. OK. Thanks, Richard. > PR tree-optimization/110252 > > gcc/ChangeLog: > > * tree-ssa-phiopt.cc (class auto_flow_sensitive): New class. > (auto_flow_sensitive::auto_flow_sensitive): New constructor. > (auto_flow_sensitive::~auto_flow_sensitive): New deconstructor. > (match_simplify_replacement): Temporarily > remove the flow sensitive info on the two statements that might > be moved. > > gcc/testsuite/ChangeLog: > > * gcc.dg/tree-ssa/phi-opt-25b.c: Updated as > __builtin_parity loses the nonzerobits info. > * gcc.c-torture/execute/pr110252-1.c: New test. > * gcc.c-torture/execute/pr110252-2.c: New test. > * gcc.c-torture/execute/pr110252-3.c: New test. > * gcc.c-torture/execute/pr110252-4.c: New test. > --- > .../gcc.c-torture/execute/pr110252-1.c | 15 ++++++ > .../gcc.c-torture/execute/pr110252-2.c | 10 ++++ > .../gcc.c-torture/execute/pr110252-3.c | 13 +++++ > .../gcc.c-torture/execute/pr110252-4.c | 8 +++ > gcc/testsuite/gcc.dg/tree-ssa/phi-opt-25b.c | 6 +-- > gcc/tree-ssa-phiopt.cc | 51 +++++++++++++++++-- > 6 files changed, 96 insertions(+), 7 deletions(-) > create mode 100644 gcc/testsuite/gcc.c-torture/execute/pr110252-1.c > create mode 100644 gcc/testsuite/gcc.c-torture/execute/pr110252-2.c > create mode 100644 gcc/testsuite/gcc.c-torture/execute/pr110252-3.c > create mode 100644 gcc/testsuite/gcc.c-torture/execute/pr110252-4.c > > diff --git a/gcc/testsuite/gcc.c-torture/execute/pr110252-1.c b/gcc/tests= uite/gcc.c-torture/execute/pr110252-1.c > new file mode 100644 > index 00000000000..4ae93ca0647 > --- /dev/null > +++ b/gcc/testsuite/gcc.c-torture/execute/pr110252-1.c > @@ -0,0 +1,15 @@ > +/* This is reduced from sel-sched.cc which was noticed was being miscomp= iled too. */ > +int g(int min_need_stall) __attribute__((__noipa__)); > +int g(int min_need_stall) > +{ > + return min_need_stall < 0 ? 1 : ((min_need_stall) < (1) ? (min_need_s= tall) : (1)); > +} > +int main(void) > +{ > + for(int i =3D -100; i <=3D 100; i++) > + { > + int t =3D g(i); > + if (t !=3D (i!=3D0)) > + __builtin_abort(); > + } > +} > diff --git a/gcc/testsuite/gcc.c-torture/execute/pr110252-2.c b/gcc/tests= uite/gcc.c-torture/execute/pr110252-2.c > new file mode 100644 > index 00000000000..7f1a7dbf134 > --- /dev/null > +++ b/gcc/testsuite/gcc.c-torture/execute/pr110252-2.c > @@ -0,0 +1,10 @@ > +signed char f() __attribute__((__noipa__)); > +signed char f() { return 0; } > +int main() > +{ > + int g =3D f() - 1; > + int e =3D g < 0 ? 1 : ((g >> (8-2))!=3D0); > + asm("":"+r"(e)); > + if (e !=3D 1) > + __builtin_abort(); > +} > diff --git a/gcc/testsuite/gcc.c-torture/execute/pr110252-3.c b/gcc/tests= uite/gcc.c-torture/execute/pr110252-3.c > new file mode 100644 > index 00000000000..c24bf1ab1e4 > --- /dev/null > +++ b/gcc/testsuite/gcc.c-torture/execute/pr110252-3.c > @@ -0,0 +1,13 @@ > + > +unsigned int a =3D 1387579096U; > +void sinkandcheck(unsigned b) __attribute__((noipa)); > +void sinkandcheck(unsigned b) > +{ > + if (a !=3D b) > + __builtin_abort(); > +} > +int main() { > + a =3D 1 < (~a) ? 1 : (~a); > + sinkandcheck(1); > + return 0; > +} > diff --git a/gcc/testsuite/gcc.c-torture/execute/pr110252-4.c b/gcc/tests= uite/gcc.c-torture/execute/pr110252-4.c > new file mode 100644 > index 00000000000..f97edd3f069 > --- /dev/null > +++ b/gcc/testsuite/gcc.c-torture/execute/pr110252-4.c > @@ -0,0 +1,8 @@ > + > +int a, b =3D 2, c =3D 2; > +int main() { > + b =3D ~(1 % (a ^ (b - (1 && c) || c & b))); > + if (b < -1) > + __builtin_abort(); > + return 0; > +} > diff --git a/gcc/testsuite/gcc.dg/tree-ssa/phi-opt-25b.c b/gcc/testsuite/= gcc.dg/tree-ssa/phi-opt-25b.c > index 7298da0c96e..0fd9b004a03 100644 > --- a/gcc/testsuite/gcc.dg/tree-ssa/phi-opt-25b.c > +++ b/gcc/testsuite/gcc.dg/tree-ssa/phi-opt-25b.c > @@ -65,8 +65,6 @@ int test_popcountll(unsigned long long x, unsigned long= long y) > return x ? __builtin_popcountll(y) : 0; > } > > -/* 3 types of functions (not including parity), each with 3 types and th= ere are 2 goto each */ > -/* { dg-final { scan-tree-dump-times "goto " 18 "optimized" } } */ > +/* 4 types of functions, each with 3 types and there are 2 goto each */ > +/* { dg-final { scan-tree-dump-times "goto " 24 "optimized" } } */ > /* { dg-final { scan-tree-dump-times "x_..D. !=3D 0" 12 "optimized" } } = */ > -/* parity case will be optimized to x!=3D0 & parity(y) . */ > -/* { dg-final { scan-tree-dump-times " & " 3 "optimized" } } */ > diff --git a/gcc/tree-ssa-phiopt.cc b/gcc/tree-ssa-phiopt.cc > index 467c9fd108a..9d542fd345f 100644 > --- a/gcc/tree-ssa-phiopt.cc > +++ b/gcc/tree-ssa-phiopt.cc > @@ -708,6 +708,45 @@ move_stmt (gimple *stmt, gimple_stmt_iterator *gsi, = auto_bitmap &inserted_exprs) > reset_flow_sensitive_info (name); > } > > +/* RAII style class to temporarily remove flow sensitive > + from ssa names defined by a gimple statement. */ > +class auto_flow_sensitive > +{ > +public: > + auto_flow_sensitive (gimple *s); > + ~auto_flow_sensitive (); > +private: > + auto_vec, 2> stack; > +}; > + > +/* Constructor for auto_flow_sensitive. Saves > + off the ssa names' flow sensitive information > + that was defined by gimple statement S and > + resets it to be non-flow based ones. */ > + > +auto_flow_sensitive::auto_flow_sensitive (gimple *s) > +{ > + if (!s) > + return; > + ssa_op_iter it; > + tree def; > + FOR_EACH_SSA_TREE_OPERAND (def, s, it, SSA_OP_DEF) > + { > + flow_sensitive_info_storage storage; > + storage.save_and_clear (def); > + stack.safe_push (std::make_pair (def, storage)); > + } > +} > + > +/* Deconstructor, restores the flow sensitive information > + for the SSA names that had been saved off. */ > + > +auto_flow_sensitive::~auto_flow_sensitive () > +{ > + for (auto p : stack) > + p.second.restore (p.first); > +} > + > /* The function match_simplify_replacement does the main work of doing = the > replacement using match and simplify. Return true if the replacemen= t is done. > Otherwise return false. > @@ -793,9 +832,15 @@ match_simplify_replacement (basic_block cond_bb, bas= ic_block middle_bb, > return false; > > tree type =3D TREE_TYPE (gimple_phi_result (phi)); > - result =3D gimple_simplify_phiopt (early_p, type, stmt, > - arg_true, arg_false, > - &seq); > + { > + auto_flow_sensitive s1(stmt_to_move); > + auto_flow_sensitive s_alt(stmt_to_move_alt); > + > + result =3D gimple_simplify_phiopt (early_p, type, stmt, > + arg_true, arg_false, > + &seq); > + } > + > if (!result) > return false; > > -- > 2.31.1 >