From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-lj1-x22f.google.com (mail-lj1-x22f.google.com [IPv6:2a00:1450:4864:20::22f]) by sourceware.org (Postfix) with ESMTPS id DCBAA3857341 for ; Thu, 15 Jun 2023 07:01:29 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org DCBAA3857341 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-x22f.google.com with SMTP id 38308e7fff4ca-2b448470602so1542741fa.2 for ; Thu, 15 Jun 2023 00:01:29 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20221208; t=1686812488; x=1689404488; 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=lAeeSIuNeNq16n3XP8/aK+mu10laavG48UcEzaUH6eU=; b=gqz7Ncplrtc4DFpW6xIZrSXw0BWXANTNI1m9XK6infRgN6M81d9PEdMivPuLwNXNU2 s2k83dXIZbcmrfkPZEjdc/UcQxSRPsxRk6n4QUrVsSj+DHeXHqFxuxU4VqnckhoVbbGv rHnJCjl06XR7kPUOTatB4jy42TGMrz1ZGluMo6kLu5jeB91xStjOQMNECSOJCF2AeLMc yO1tV/pQnz8udLs4Hagfoj1SLgz920pJ1K16tydAw6HHQNSiQxHOrpq9GQbWWw7+ONk6 0e1d694QryYZXJRcebi10NUZyV5KU2VkAcSCT1Mo9uolxWQ4G33i0Tw1VeeUWc8W283/ ml4Q== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20221208; t=1686812488; x=1689404488; 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=lAeeSIuNeNq16n3XP8/aK+mu10laavG48UcEzaUH6eU=; b=S95X/TFdCy5G7q/Hh3hVdYBu6njYFhy+CRgQL8HQEQZzg/k3woa6cI8OKbKlzATsDQ YnfU0c/FAFbUpsOjy0XSWHMlxmtvHXImjMaS8XiEWdTYTUx5Mx1COOUoHfwRllryLSAC x1s14VdzYmf3f+sgWTeLV9mkQKU+xVjGJSPsW5JL+52TsDu5nvX9ey3cCWGBMDHY/Xx0 c0BXIVyV6SfhW0Y/w+Zug+dGkeVmIwFLjnUiIE9drv7D42/xUBElGkXuhKxJwDX+4kBf JhRRaAHL4gOX7Bw/t5xMC3q/h/W0GzY/ioFPG114OFEcsH788k+S6tTXZjX8zily2NUu ikzQ== X-Gm-Message-State: AC+VfDwLwr6t/5WcRQ0Oq8QRKS/vru9eEgh04OZ4DibS9BuplRO8EsDn n3mO3pRa1TJ4dezaLrM32o2mHLdBteuvr9segKc= X-Google-Smtp-Source: ACHHUZ7yi66i731dWg4G0XdVhB2Ql4hTS/ARrC4IYY71bWIel3eOTMLcyPhrOVD9BOt1YKtn0ajVVgzLvFqUrGstL/w= X-Received: by 2002:a2e:360e:0:b0:2b1:a8bb:978f with SMTP id d14-20020a2e360e000000b002b1a8bb978fmr8350347lja.9.1686812487764; Thu, 15 Jun 2023 00:01:27 -0700 (PDT) MIME-Version: 1.0 References: <20230615061704.3514834-1-apinski@marvell.com> In-Reply-To: <20230615061704.3514834-1-apinski@marvell.com> From: Richard Biener Date: Thu, 15 Jun 2023 08:58:58 +0200 Message-ID: Subject: Re: [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 Thu, Jun 15, 2023 at 8:18=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. > > So the solution here is to temporarily remove the flow sensitive info of = the statements > phiopt might move and try match only then. This is done via the new class= auto_flow_sensitive. > > OK? Bootstrapped and tested on x86_64-linux-gnu with no regressions. Ah, looks like I forgot about pt->null in the approach used by maybe_fold_comparisons_from_match_pd. That also has much less control over which stmts are possibly under conditional control so your approach might be simpler for phiopt. Note the flow-sensitive info is on SSA defs and some stmts have multiple of those so I think the auto_ class should construct on SSA defs instead. Can you make it a non-auto (POD?) class so I can possibly re-use = it in the maybe_fold_comparisons_from_match_pd machinery though? Like class flow_sensitive_info_storage { public: save (tree); save_and_clear (tree); restore (tree); private: ... members ... }; ? > PR tree-optimization/110252 > > gcc/ChangeLog: > > * tree-ssa-phiopt.cc (match_simplify_replacement): Temporarily > remove the flow sensitive info on the two statements that might > be moved. > * tree-ssanames.cc (auto_flow_sensitive::auto_flow_sensitive): Ne= w method. > (auto_flow_sensitive::~auto_flow_sensitive): New method. > * tree-ssanames.h (class auto_flow_sensitive): New class. > > gcc/testsuite/ChangeLog: > > * gcc.c-torture/execute/pr110252.c: New test. > * gcc.c-torture/execute/pr110252-1.c: New test. > * gcc.dg/tree-ssa/phi-opt-25b.c: Updated as > __builtin_parity loses the nonzerobits info. > --- > .../gcc.c-torture/execute/pr110252-1.c | 15 +++++++ > .../gcc.c-torture/execute/pr110252.c | 10 +++++ > gcc/testsuite/gcc.dg/tree-ssa/phi-opt-25b.c | 6 +-- > gcc/tree-ssa-phiopt.cc | 12 ++++-- > gcc/tree-ssanames.cc | 40 +++++++++++++++++++ > gcc/tree-ssanames.h | 21 ++++++++++ > 6 files changed, 97 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.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.c b/gcc/testsui= te/gcc.c-torture/execute/pr110252.c > new file mode 100644 > index 00000000000..7f1a7dbf134 > --- /dev/null > +++ b/gcc/testsuite/gcc.c-torture/execute/pr110252.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.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 d9a1153086f..5f871e019e4 100644 > --- a/gcc/tree-ssa-phiopt.cc > +++ b/gcc/tree-ssa-phiopt.cc > @@ -782,9 +782,15 @@ match_simplify_replacement (basic_block cond_bb, bas= ic_block middle_bb, > } > > 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; > > diff --git a/gcc/tree-ssanames.cc b/gcc/tree-ssanames.cc > index 5fdb6a37e9f..765d343ba49 100644 > --- a/gcc/tree-ssanames.cc > +++ b/gcc/tree-ssanames.cc > @@ -812,6 +812,46 @@ reset_flow_sensitive_info_in_bb (basic_block bb) > } > } > > +/* Constructor for auto_flow_sensitive. Saves > + off the ssa name's 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; > + > + name =3D gimple_get_lhs (s); > + > + if (!POINTER_TYPE_P (TREE_TYPE (name))) > + { > + range_info =3D SSA_NAME_RANGE_INFO (name); > + SSA_NAME_RANGE_INFO (name) =3D NULL; > + } > + else if (SSA_NAME_PTR_INFO (name)) > + { > + get_ptr_info_alignment (SSA_NAME_PTR_INFO (name), &align, &misalig= n); > + null =3D SSA_NAME_PTR_INFO (name)->pt.null; > + } > +} > + > +/* Deconstructor, restores the flow sensitive information > + for the SSA name. */ > +auto_flow_sensitive::~auto_flow_sensitive () > +{ > + if (!name) > + return; > + > + if (!POINTER_TYPE_P (TREE_TYPE (name))) > + SSA_NAME_RANGE_INFO (name) =3D range_info; > + else if (SSA_NAME_PTR_INFO (name)) > + { > + if (align) > + set_ptr_info_alignment (SSA_NAME_PTR_INFO (name), align, misalign= ); > + SSA_NAME_PTR_INFO (name)->pt.null =3D null; > + } > +} > + > /* Release all the SSA_NAMEs created by STMT. */ > > void > diff --git a/gcc/tree-ssanames.h b/gcc/tree-ssanames.h > index f3fa609208a..28b83b779fa 100644 > --- a/gcc/tree-ssanames.h > +++ b/gcc/tree-ssanames.h > @@ -137,4 +137,25 @@ make_temp_ssa_name (tree type, gimple *stmt, const c= har *name) > } > > > +/* RAII style class to temporarily remove flow sensitive > + from a ssa name defined by a gimple statement. > + An usage example is inside phiopt, where using match-and-simplify > + but since the statement is from inside a conditional bb, the > + flow sensitive information can't be used. */ > +class auto_flow_sensitive > +{ > +public: > + auto_flow_sensitive (gimple *s); > + ~auto_flow_sensitive (); > +private: > + tree name =3D nullptr; > + /* The range info for non pointers */ > + vrange_storage *range_info; > + /* Flow sensitive pointer information. */ > + unsigned int align =3D 0; > + unsigned int misalign =3D 0; > + bool null =3D false; > +}; > + > + > #endif /* GCC_TREE_SSANAMES_H */ > -- > 2.31.1 >