From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mx0b-0016f401.pphosted.com (mx0b-0016f401.pphosted.com [67.231.156.173]) by sourceware.org (Postfix) with ESMTPS id B58BC3858CDB for ; Sat, 15 Jul 2023 03:20:50 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org B58BC3858CDB Authentication-Results: sourceware.org; dmarc=pass (p=none dis=none) header.from=marvell.com Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=marvell.com Received: from pps.filterd (m0045851.ppops.net [127.0.0.1]) by mx0b-0016f401.pphosted.com (8.17.1.19/8.17.1.19) with ESMTP id 36F2nxVY006766 for ; Fri, 14 Jul 2023 20:20:50 -0700 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=marvell.com; h=from : to : cc : subject : date : message-id : in-reply-to : references : mime-version : content-transfer-encoding : content-type; s=pfpt0220; bh=4jROPOKTgcXBcPUpUM1w9T4pVyc6ZEcugagBzSASBL8=; b=HNfEovBIrarY/IXZbZQe7ExhlkwnRsHptYl+yIw7O1bmXVAdrrXbt5eLkVDcjBE6rFRW sfoA8CjFMKziytmJRsNsADhQQkx8x8FhGpo2Bf42/Ym+F1D/6Ml2YHMSM3oAWTUROQ+y wcRmVTt65kUBRDbx8wOxMFOfb1u6uM0OMLyIdHwqWRC3ys83TBLyYGg9qYFD2uvKHn54 vRw0j1wKYQLsQ8nHDUfm/Qn5ZcZ+pYAYB+IUssz06St3GQjL5eI/zRQzXCI5HSADmXtl blVaMlIyhAYqESbyFHuNWH/FogL3f7gZHJ0uKE+ocKRS+pHAKq2y4DrOyLUHbLD4k+eT Hg== Received: from dc5-exch02.marvell.com ([199.233.59.182]) by mx0b-0016f401.pphosted.com (PPS) with ESMTPS id 3rtptxcgf2-1 (version=TLSv1.2 cipher=ECDHE-RSA-AES256-SHA384 bits=256 verify=NOT) for ; Fri, 14 Jul 2023 20:20:49 -0700 Received: from DC5-EXCH02.marvell.com (10.69.176.39) by DC5-EXCH02.marvell.com (10.69.176.39) with Microsoft SMTP Server (TLS) id 15.0.1497.48; Fri, 14 Jul 2023 20:20:47 -0700 Received: from maili.marvell.com (10.69.176.80) by DC5-EXCH02.marvell.com (10.69.176.39) with Microsoft SMTP Server id 15.0.1497.48 via Frontend Transport; Fri, 14 Jul 2023 20:20:47 -0700 Received: from vpnclient.wrightpinski.org.com (unknown [10.76.242.112]) by maili.marvell.com (Postfix) with ESMTP id CE6195B6939; Fri, 14 Jul 2023 20:20:45 -0700 (PDT) From: Andrew Pinski To: CC: Andrew Pinski Subject: [PATCH 2/2] [PATCH] Fix tree-opt/110252: wrong code due to phiopt using flow sensitive info during match Date: Fri, 14 Jul 2023 20:19:57 -0700 Message-ID: <20230715031957.1147225-2-apinski@marvell.com> X-Mailer: git-send-email 2.31.1 In-Reply-To: <20230715031957.1147225-1-apinski@marvell.com> References: <20230715031957.1147225-1-apinski@marvell.com> MIME-Version: 1.0 Content-Transfer-Encoding: 8bit Content-Type: text/plain X-Proofpoint-GUID: 4INNeZWv1q2Xj7SUdMFiA7qhP27KkiU1 X-Proofpoint-ORIG-GUID: 4INNeZWv1q2Xj7SUdMFiA7qhP27KkiU1 X-Proofpoint-Virus-Version: vendor=baseguard engine=ICAP:2.0.254,Aquarius:18.0.957,Hydra:6.0.591,FMLib:17.11.176.26 definitions=2023-07-14_12,2023-07-13_01,2023-05-22_02 X-Spam-Status: No, score=-14.6 required=5.0 tests=BAYES_00,DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,GIT_PATCH_0,RCVD_IN_DNSWL_LOW,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: 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 information on it based on the condition that is for the COND_EXPR but that might create wrong 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. 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/testsuite/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 miscompiled 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_stall) : (1)); +} +int main(void) +{ + for(int i = -100; i <= 100; i++) + { + int t = g(i); + if (t != (i!=0)) + __builtin_abort(); + } +} diff --git a/gcc/testsuite/gcc.c-torture/execute/pr110252-2.c b/gcc/testsuite/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 = f() - 1; + int e = g < 0 ? 1 : ((g >> (8-2))!=0); + asm("":"+r"(e)); + if (e != 1) + __builtin_abort(); +} diff --git a/gcc/testsuite/gcc.c-torture/execute/pr110252-3.c b/gcc/testsuite/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 = 1387579096U; +void sinkandcheck(unsigned b) __attribute__((noipa)); +void sinkandcheck(unsigned b) +{ + if (a != b) + __builtin_abort(); +} +int main() { + a = 1 < (~a) ? 1 : (~a); + sinkandcheck(1); + return 0; +} diff --git a/gcc/testsuite/gcc.c-torture/execute/pr110252-4.c b/gcc/testsuite/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 = 2, c = 2; +int main() { + b = ~(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 there 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. != 0" 12 "optimized" } } */ -/* parity case will be optimized to x!=0 & 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 replacement is done. Otherwise return false. @@ -793,9 +832,15 @@ match_simplify_replacement (basic_block cond_bb, basic_block middle_bb, return false; tree type = TREE_TYPE (gimple_phi_result (phi)); - result = 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 = gimple_simplify_phiopt (early_p, type, stmt, + arg_true, arg_false, + &seq); + } + if (!result) return false; -- 2.31.1