Message ID | mptwnixp3ku.fsf@arm.com |
---|---|
State | New |
Headers |
Return-Path: <gcc-patches-bounces+patchwork=sourceware.org@gcc.gnu.org> X-Original-To: patchwork@sourceware.org Delivered-To: patchwork@sourceware.org Received: from server2.sourceware.org (localhost [IPv6:::1]) by sourceware.org (Postfix) with ESMTP id 45FE53857C5D for <patchwork@sourceware.org>; Tue, 18 Jan 2022 13:40:48 +0000 (GMT) DKIM-Filter: OpenDKIM Filter v2.11.0 sourceware.org 45FE53857C5D DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gcc.gnu.org; s=default; t=1642513248; bh=9fL2IGY+c2ossGFj+VP15mFpBcDO2nYnSwHUyOedl80=; h=To:Subject:Date:List-Id:List-Unsubscribe:List-Archive:List-Post: List-Help:List-Subscribe:From:Reply-To:Cc:From; b=Q+rKMgx8Wf8WiCHRSK+eLy696VX7XSbKvdprxm3UWAw7sFf4IvKEwcNNzh8YEAXwT jO4Lk75MEbu7bpEayrNWTUa/5sh9oEeUsOeuAVJLfIXNWScEFdwJ9Q6gtzxzB88adH OgQwfjdVg5FTsjdnnBb2ST2CbJq2vypmQWqseNMM= X-Original-To: gcc-patches@gcc.gnu.org Delivered-To: gcc-patches@gcc.gnu.org Received: from foss.arm.com (foss.arm.com [217.140.110.172]) by sourceware.org (Postfix) with ESMTP id 2DEE4385802C; Tue, 18 Jan 2022 13:37:39 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org 2DEE4385802C Received: from usa-sjc-imap-foss1.foss.arm.com (unknown [10.121.207.14]) by usa-sjc-mx-foss1.foss.arm.com (Postfix) with ESMTP id D13FD1FB; Tue, 18 Jan 2022 05:37:38 -0800 (PST) Received: from localhost (unknown [10.32.98.88]) by usa-sjc-imap-foss1.foss.arm.com (Postfix) with ESMTPSA id 5BE743F73D; Tue, 18 Jan 2022 05:37:38 -0800 (PST) To: gcc-patches@gcc.gnu.org Mail-Followup-To: gcc-patches@gcc.gnu.org, msebor@gcc.gnu.org, richard.sandiford@arm.com Subject: [PATCH] waccess: Look at calls when tracking clobbers [PR104092] Date: Tue, 18 Jan 2022 13:37:37 +0000 Message-ID: <mptwnixp3ku.fsf@arm.com> User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/26.3 (gnu/linux) MIME-Version: 1.0 Content-Type: text/plain X-Spam-Status: No, score=-12.4 required=5.0 tests=BAYES_00, GIT_PATCH_0, KAM_DMARC_STATUS, KAM_SHORT, SPF_HELO_NONE, SPF_PASS, TXREP autolearn=ham autolearn_force=no version=3.4.4 X-Spam-Checker-Version: SpamAssassin 3.4.4 (2020-01-24) on server2.sourceware.org X-BeenThere: gcc-patches@gcc.gnu.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Gcc-patches mailing list <gcc-patches.gcc.gnu.org> List-Unsubscribe: <https://gcc.gnu.org/mailman/options/gcc-patches>, <mailto:gcc-patches-request@gcc.gnu.org?subject=unsubscribe> List-Archive: <https://gcc.gnu.org/pipermail/gcc-patches/> List-Post: <mailto:gcc-patches@gcc.gnu.org> List-Help: <mailto:gcc-patches-request@gcc.gnu.org?subject=help> List-Subscribe: <https://gcc.gnu.org/mailman/listinfo/gcc-patches>, <mailto:gcc-patches-request@gcc.gnu.org?subject=subscribe> From: Richard Sandiford via Gcc-patches <gcc-patches@gcc.gnu.org> Reply-To: Richard Sandiford <richard.sandiford@arm.com> Cc: msebor@gcc.gnu.org Errors-To: gcc-patches-bounces+patchwork=sourceware.org@gcc.gnu.org Sender: "Gcc-patches" <gcc-patches-bounces+patchwork=sourceware.org@gcc.gnu.org> |
Series |
waccess: Look at calls when tracking clobbers [PR104092]
|
|
Commit Message
Richard Sandiford
Jan. 18, 2022, 1:37 p.m. UTC
In this PR the waccess pass was fed: D.10779 ={v} {CLOBBER}; VIEW_CONVERT_EXPR<svuint64_t[2]>(D.10779) = .MASK_LOAD_LANES (addr_5(D), 64B, _2); _7 = D.10779.__val[0]; However, the tracking of m_clobbers only looked at gassigns, so it missed that the clobber on the first line was overwritten by the call on the second line. This patch splits the updating of m_clobbers out into its own function, called after the check_*() routines, and extends it to handle both gassigns and gcalls. I think that makes sense as an instance of the "read, operate, write" model, with the new function being part of "write". Previously only the gimple_clobber_p handling was conditional on m_check_dangling_p, but I think the whole of the new function can be. We only enter stmts into m_clobbers if m_check_dangling_p, so we only need to remove them under the same condition. Tested on aarch64-linux-gnu. OK to install? Richard gcc/ PR middle-end/104092 * gimple-ssa-warn-access.cc (pass_waccess::update_clobbers_from_lhs): New function, split out from... (pass_waccess::check_stmt): ...here and generalized to calls. (pass_waccess::check_block): Call it. gcc/testsuite/ * gcc.target/aarch64/sve/acle/general/pr104092.c: New test. --- gcc/gimple-ssa-warn-access.cc | 68 +++++++++++-------- .../aarch64/sve/acle/general/pr104092.c | 7 ++ 2 files changed, 48 insertions(+), 27 deletions(-) create mode 100644 gcc/testsuite/gcc.target/aarch64/sve/acle/general/pr104092.c
Comments
On 1/18/22 06:37, Richard Sandiford wrote: > In this PR the waccess pass was fed: > > D.10779 ={v} {CLOBBER}; > VIEW_CONVERT_EXPR<svuint64_t[2]>(D.10779) = .MASK_LOAD_LANES (addr_5(D), 64B, _2); > _7 = D.10779.__val[0]; > > However, the tracking of m_clobbers only looked at gassigns, > so it missed that the clobber on the first line was overwritten > by the call on the second line. > > This patch splits the updating of m_clobbers out into its own > function, called after the check_*() routines, and extends it > to handle both gassigns and gcalls. I think that makes sense > as an instance of the "read, operate, write" model, with the > new function being part of "write". > > Previously only the gimple_clobber_p handling was conditional > on m_check_dangling_p, but I think the whole of the new function > can be. We only enter stmts into m_clobbers if m_check_dangling_p, > so we only need to remove them under the same condition. Thanks for the patch. If you or someone can think of a test case that's independent of a target, adding one would be very helpful. Other than that, since I can't approve any changes I CC Jason who was kind enough to approve the implementation of the warning for his OK. Martin > > Tested on aarch64-linux-gnu. OK to install? > > Richard > > > gcc/ > PR middle-end/104092 > * gimple-ssa-warn-access.cc (pass_waccess::update_clobbers_from_lhs): > New function, split out from... > (pass_waccess::check_stmt): ...here and generalized to calls. > (pass_waccess::check_block): Call it. > > gcc/testsuite/ > * gcc.target/aarch64/sve/acle/general/pr104092.c: New test. > --- > gcc/gimple-ssa-warn-access.cc | 68 +++++++++++-------- > .../aarch64/sve/acle/general/pr104092.c | 7 ++ > 2 files changed, 48 insertions(+), 27 deletions(-) > create mode 100644 gcc/testsuite/gcc.target/aarch64/sve/acle/general/pr104092.c > > diff --git a/gcc/gimple-ssa-warn-access.cc b/gcc/gimple-ssa-warn-access.cc > index f639807a78a..25066fa6b89 100644 > --- a/gcc/gimple-ssa-warn-access.cc > +++ b/gcc/gimple-ssa-warn-access.cc > @@ -2094,6 +2094,9 @@ private: > /* Check a non-call statement. */ > void check_stmt (gimple *); > > + /* Update the clobber map based on the lhs of a statement. */ > + void update_clobbers_from_lhs (gimple *); > + > /* Check statements in a basic block. */ > void check_block (basic_block); > > @@ -4270,33 +4273,6 @@ is_auto_decl (tree x) > void > pass_waccess::check_stmt (gimple *stmt) > { > - if (m_check_dangling_p && gimple_clobber_p (stmt)) > - { > - /* Ignore clobber statemts in blocks with exceptional edges. */ > - basic_block bb = gimple_bb (stmt); > - edge e = EDGE_PRED (bb, 0); > - if (e->flags & EDGE_EH) > - return; > - > - tree var = gimple_assign_lhs (stmt); > - m_clobbers.put (var, stmt); > - return; > - } > - > - if (is_gimple_assign (stmt)) > - { > - /* Clobbered unnamed temporaries such as compound literals can be > - revived. Check for an assignment to one and remove it from > - M_CLOBBERS. */ > - tree lhs = gimple_assign_lhs (stmt); > - while (handled_component_p (lhs)) > - lhs = TREE_OPERAND (lhs, 0); > - > - if (is_auto_decl (lhs)) > - m_clobbers.remove (lhs); > - return; > - } > - > if (greturn *ret = dyn_cast <greturn *> (stmt)) > { > if (optimize && flag_isolate_erroneous_paths_dereference) > @@ -4326,6 +4302,42 @@ pass_waccess::check_stmt (gimple *stmt) > } > } > > +/* Update the clobber map based on the lhs of STMT. */ > + > +void > +pass_waccess::update_clobbers_from_lhs (gimple *stmt) > +{ > + if (gimple_clobber_p (stmt)) > + { > + /* Ignore clobber statements in blocks with exceptional edges. */ > + basic_block bb = gimple_bb (stmt); > + edge e = EDGE_PRED (bb, 0); > + if (e->flags & EDGE_EH) > + return; > + > + tree var = gimple_assign_lhs (stmt); > + m_clobbers.put (var, stmt); > + return; > + } > + > + if (is_gimple_assign (stmt) || is_gimple_call (stmt)) > + { > + /* Clobbered unnamed temporaries such as compound literals can be > + revived. Check for an assignment to one and remove it from > + M_CLOBBERS. */ > + tree lhs = gimple_get_lhs (stmt); > + if (!lhs) > + return; > + > + while (handled_component_p (lhs)) > + lhs = TREE_OPERAND (lhs, 0); > + > + if (is_auto_decl (lhs)) > + m_clobbers.remove (lhs); > + return; > + } > +} > + > /* Check basic block BB for invalid accesses. */ > > void > @@ -4340,6 +4352,8 @@ pass_waccess::check_block (basic_block bb) > check_call (call); > else > check_stmt (stmt); > + if (m_check_dangling_p) > + update_clobbers_from_lhs (stmt); > } > } > > diff --git a/gcc/testsuite/gcc.target/aarch64/sve/acle/general/pr104092.c b/gcc/testsuite/gcc.target/aarch64/sve/acle/general/pr104092.c > new file mode 100644 > index 00000000000..c17ece7d82f > --- /dev/null > +++ b/gcc/testsuite/gcc.target/aarch64/sve/acle/general/pr104092.c > @@ -0,0 +1,7 @@ > +/* { dg-options "-O2 -Wall" } */ > + > +#include <arm_sve.h> > + > +svuint64_t bar(svbool_t pg, const uint64_t *addr) { > + return svget2(svld2_u64(pg, addr), 0); > +}
On 1/18/22 13:06, Martin Sebor wrote: > On 1/18/22 06:37, Richard Sandiford wrote: >> In this PR the waccess pass was fed: >> >> D.10779 ={v} {CLOBBER}; >> VIEW_CONVERT_EXPR<svuint64_t[2]>(D.10779) = .MASK_LOAD_LANES >> (addr_5(D), 64B, _2); >> _7 = D.10779.__val[0]; >> >> However, the tracking of m_clobbers only looked at gassigns, >> so it missed that the clobber on the first line was overwritten >> by the call on the second line. >> >> This patch splits the updating of m_clobbers out into its own >> function, called after the check_*() routines, and extends it >> to handle both gassigns and gcalls. I think that makes sense >> as an instance of the "read, operate, write" model, with the >> new function being part of "write". >> >> Previously only the gimple_clobber_p handling was conditional >> on m_check_dangling_p, but I think the whole of the new function >> can be. We only enter stmts into m_clobbers if m_check_dangling_p, >> so we only need to remove them under the same condition. > > Thanks for the patch. If you or someone can think of a test case > that's independent of a target, adding one would be very helpful. > > Other than that, since I can't approve any changes I CC Jason who > was kind enough to approve the implementation of the warning for > his OK. OK if Martin is happy with it. >> Tested on aarch64-linux-gnu. OK to install? >> >> Richard >> >> >> gcc/ >> PR middle-end/104092 >> * gimple-ssa-warn-access.cc (pass_waccess::update_clobbers_from_lhs): >> New function, split out from... >> (pass_waccess::check_stmt): ...here and generalized to calls. >> (pass_waccess::check_block): Call it. >> >> gcc/testsuite/ >> * gcc.target/aarch64/sve/acle/general/pr104092.c: New test. >> --- >> gcc/gimple-ssa-warn-access.cc | 68 +++++++++++-------- >> .../aarch64/sve/acle/general/pr104092.c | 7 ++ >> 2 files changed, 48 insertions(+), 27 deletions(-) >> create mode 100644 >> gcc/testsuite/gcc.target/aarch64/sve/acle/general/pr104092.c >> >> diff --git a/gcc/gimple-ssa-warn-access.cc >> b/gcc/gimple-ssa-warn-access.cc >> index f639807a78a..25066fa6b89 100644 >> --- a/gcc/gimple-ssa-warn-access.cc >> +++ b/gcc/gimple-ssa-warn-access.cc >> @@ -2094,6 +2094,9 @@ private: >> /* Check a non-call statement. */ >> void check_stmt (gimple *); >> + /* Update the clobber map based on the lhs of a statement. */ >> + void update_clobbers_from_lhs (gimple *); >> + >> /* Check statements in a basic block. */ >> void check_block (basic_block); >> @@ -4270,33 +4273,6 @@ is_auto_decl (tree x) >> void >> pass_waccess::check_stmt (gimple *stmt) >> { >> - if (m_check_dangling_p && gimple_clobber_p (stmt)) >> - { >> - /* Ignore clobber statemts in blocks with exceptional edges. */ >> - basic_block bb = gimple_bb (stmt); >> - edge e = EDGE_PRED (bb, 0); >> - if (e->flags & EDGE_EH) >> - return; >> - >> - tree var = gimple_assign_lhs (stmt); >> - m_clobbers.put (var, stmt); >> - return; >> - } >> - >> - if (is_gimple_assign (stmt)) >> - { >> - /* Clobbered unnamed temporaries such as compound literals can be >> - revived. Check for an assignment to one and remove it from >> - M_CLOBBERS. */ >> - tree lhs = gimple_assign_lhs (stmt); >> - while (handled_component_p (lhs)) >> - lhs = TREE_OPERAND (lhs, 0); >> - >> - if (is_auto_decl (lhs)) >> - m_clobbers.remove (lhs); >> - return; >> - } >> - >> if (greturn *ret = dyn_cast <greturn *> (stmt)) >> { >> if (optimize && flag_isolate_erroneous_paths_dereference) >> @@ -4326,6 +4302,42 @@ pass_waccess::check_stmt (gimple *stmt) >> } >> } >> +/* Update the clobber map based on the lhs of STMT. */ >> + >> +void >> +pass_waccess::update_clobbers_from_lhs (gimple *stmt) >> +{ >> + if (gimple_clobber_p (stmt)) >> + { >> + /* Ignore clobber statements in blocks with exceptional edges. */ >> + basic_block bb = gimple_bb (stmt); >> + edge e = EDGE_PRED (bb, 0); >> + if (e->flags & EDGE_EH) >> + return; >> + >> + tree var = gimple_assign_lhs (stmt); >> + m_clobbers.put (var, stmt); >> + return; >> + } >> + >> + if (is_gimple_assign (stmt) || is_gimple_call (stmt)) >> + { >> + /* Clobbered unnamed temporaries such as compound literals can be >> + revived. Check for an assignment to one and remove it from >> + M_CLOBBERS. */ >> + tree lhs = gimple_get_lhs (stmt); >> + if (!lhs) >> + return; >> + >> + while (handled_component_p (lhs)) >> + lhs = TREE_OPERAND (lhs, 0); >> + >> + if (is_auto_decl (lhs)) >> + m_clobbers.remove (lhs); >> + return; >> + } >> +} >> + >> /* Check basic block BB for invalid accesses. */ >> void >> @@ -4340,6 +4352,8 @@ pass_waccess::check_block (basic_block bb) >> check_call (call); >> else >> check_stmt (stmt); >> + if (m_check_dangling_p) >> + update_clobbers_from_lhs (stmt); >> } >> } >> diff --git >> a/gcc/testsuite/gcc.target/aarch64/sve/acle/general/pr104092.c >> b/gcc/testsuite/gcc.target/aarch64/sve/acle/general/pr104092.c >> new file mode 100644 >> index 00000000000..c17ece7d82f >> --- /dev/null >> +++ b/gcc/testsuite/gcc.target/aarch64/sve/acle/general/pr104092.c >> @@ -0,0 +1,7 @@ >> +/* { dg-options "-O2 -Wall" } */ >> + >> +#include <arm_sve.h> >> + >> +svuint64_t bar(svbool_t pg, const uint64_t *addr) { >> + return svget2(svld2_u64(pg, addr), 0); >> +} >
On Tue, Jan 18, 2022 at 2:40 PM Richard Sandiford via Gcc-patches <gcc-patches@gcc.gnu.org> wrote: > > In this PR the waccess pass was fed: > > D.10779 ={v} {CLOBBER}; > VIEW_CONVERT_EXPR<svuint64_t[2]>(D.10779) = .MASK_LOAD_LANES (addr_5(D), 64B, _2); > _7 = D.10779.__val[0]; > > However, the tracking of m_clobbers only looked at gassigns, > so it missed that the clobber on the first line was overwritten > by the call on the second line. Just as a note another possible def can come via asm() outputs and clobbers. There would have been walk_stmt_load_store_ops to track all those down (not sure if the function is a good fit here). > This patch splits the updating of m_clobbers out into its own > function, called after the check_*() routines, and extends it > to handle both gassigns and gcalls. I think that makes sense > as an instance of the "read, operate, write" model, with the > new function being part of "write". > > Previously only the gimple_clobber_p handling was conditional > on m_check_dangling_p, but I think the whole of the new function > can be. We only enter stmts into m_clobbers if m_check_dangling_p, > so we only need to remove them under the same condition. > > Tested on aarch64-linux-gnu. OK to install? > > Richard > > > gcc/ > PR middle-end/104092 > * gimple-ssa-warn-access.cc (pass_waccess::update_clobbers_from_lhs): > New function, split out from... > (pass_waccess::check_stmt): ...here and generalized to calls. > (pass_waccess::check_block): Call it. > > gcc/testsuite/ > * gcc.target/aarch64/sve/acle/general/pr104092.c: New test. > --- > gcc/gimple-ssa-warn-access.cc | 68 +++++++++++-------- > .../aarch64/sve/acle/general/pr104092.c | 7 ++ > 2 files changed, 48 insertions(+), 27 deletions(-) > create mode 100644 gcc/testsuite/gcc.target/aarch64/sve/acle/general/pr104092.c > > diff --git a/gcc/gimple-ssa-warn-access.cc b/gcc/gimple-ssa-warn-access.cc > index f639807a78a..25066fa6b89 100644 > --- a/gcc/gimple-ssa-warn-access.cc > +++ b/gcc/gimple-ssa-warn-access.cc > @@ -2094,6 +2094,9 @@ private: > /* Check a non-call statement. */ > void check_stmt (gimple *); > > + /* Update the clobber map based on the lhs of a statement. */ > + void update_clobbers_from_lhs (gimple *); > + > /* Check statements in a basic block. */ > void check_block (basic_block); > > @@ -4270,33 +4273,6 @@ is_auto_decl (tree x) > void > pass_waccess::check_stmt (gimple *stmt) > { > - if (m_check_dangling_p && gimple_clobber_p (stmt)) > - { > - /* Ignore clobber statemts in blocks with exceptional edges. */ > - basic_block bb = gimple_bb (stmt); > - edge e = EDGE_PRED (bb, 0); > - if (e->flags & EDGE_EH) > - return; > - > - tree var = gimple_assign_lhs (stmt); > - m_clobbers.put (var, stmt); > - return; > - } > - > - if (is_gimple_assign (stmt)) > - { > - /* Clobbered unnamed temporaries such as compound literals can be > - revived. Check for an assignment to one and remove it from > - M_CLOBBERS. */ > - tree lhs = gimple_assign_lhs (stmt); > - while (handled_component_p (lhs)) > - lhs = TREE_OPERAND (lhs, 0); > - > - if (is_auto_decl (lhs)) > - m_clobbers.remove (lhs); > - return; > - } > - > if (greturn *ret = dyn_cast <greturn *> (stmt)) > { > if (optimize && flag_isolate_erroneous_paths_dereference) > @@ -4326,6 +4302,42 @@ pass_waccess::check_stmt (gimple *stmt) > } > } > > +/* Update the clobber map based on the lhs of STMT. */ > + > +void > +pass_waccess::update_clobbers_from_lhs (gimple *stmt) > +{ > + if (gimple_clobber_p (stmt)) > + { > + /* Ignore clobber statements in blocks with exceptional edges. */ > + basic_block bb = gimple_bb (stmt); > + edge e = EDGE_PRED (bb, 0); > + if (e->flags & EDGE_EH) > + return; > + > + tree var = gimple_assign_lhs (stmt); > + m_clobbers.put (var, stmt); > + return; > + } > + > + if (is_gimple_assign (stmt) || is_gimple_call (stmt)) > + { > + /* Clobbered unnamed temporaries such as compound literals can be > + revived. Check for an assignment to one and remove it from > + M_CLOBBERS. */ > + tree lhs = gimple_get_lhs (stmt); > + if (!lhs) > + return; > + > + while (handled_component_p (lhs)) > + lhs = TREE_OPERAND (lhs, 0); > + > + if (is_auto_decl (lhs)) > + m_clobbers.remove (lhs); > + return; > + } > +} > + > /* Check basic block BB for invalid accesses. */ > > void > @@ -4340,6 +4352,8 @@ pass_waccess::check_block (basic_block bb) > check_call (call); > else > check_stmt (stmt); > + if (m_check_dangling_p) > + update_clobbers_from_lhs (stmt); > } > } > > diff --git a/gcc/testsuite/gcc.target/aarch64/sve/acle/general/pr104092.c b/gcc/testsuite/gcc.target/aarch64/sve/acle/general/pr104092.c > new file mode 100644 > index 00000000000..c17ece7d82f > --- /dev/null > +++ b/gcc/testsuite/gcc.target/aarch64/sve/acle/general/pr104092.c > @@ -0,0 +1,7 @@ > +/* { dg-options "-O2 -Wall" } */ > + > +#include <arm_sve.h> > + > +svuint64_t bar(svbool_t pg, const uint64_t *addr) { > + return svget2(svld2_u64(pg, addr), 0); > +} > -- > 2.25.1 >
Richard Biener <richard.guenther@gmail.com> writes: > On Tue, Jan 18, 2022 at 2:40 PM Richard Sandiford via Gcc-patches > <gcc-patches@gcc.gnu.org> wrote: >> >> In this PR the waccess pass was fed: >> >> D.10779 ={v} {CLOBBER}; >> VIEW_CONVERT_EXPR<svuint64_t[2]>(D.10779) = .MASK_LOAD_LANES (addr_5(D), 64B, _2); >> _7 = D.10779.__val[0]; >> >> However, the tracking of m_clobbers only looked at gassigns, >> so it missed that the clobber on the first line was overwritten >> by the call on the second line. > > Just as a note another possible def can come via asm() outputs > and clobbers. There would have been walk_stmt_load_store_ops > to track all those down (not sure if the function is a good fit here). Hmm. Looking at what the pass is doing in more detail, I'm not sure this approach to handling m_clobbers is safe. The pass walks the blocks in sequence (rather than using a dom walk, say): FOR_EACH_BB_FN (bb, fun) check_block (bb); so it could see the clobber after a later dominating assignment. Similarly check_call_dangling could see a use that is “protected” by a later assignment. Richard
On 1/19/22 03:09, Richard Sandiford wrote: > Richard Biener <richard.guenther@gmail.com> writes: >> On Tue, Jan 18, 2022 at 2:40 PM Richard Sandiford via Gcc-patches >> <gcc-patches@gcc.gnu.org> wrote: >>> >>> In this PR the waccess pass was fed: >>> >>> D.10779 ={v} {CLOBBER}; >>> VIEW_CONVERT_EXPR<svuint64_t[2]>(D.10779) = .MASK_LOAD_LANES (addr_5(D), 64B, _2); >>> _7 = D.10779.__val[0]; >>> >>> However, the tracking of m_clobbers only looked at gassigns, >>> so it missed that the clobber on the first line was overwritten >>> by the call on the second line. >> >> Just as a note another possible def can come via asm() outputs >> and clobbers. There would have been walk_stmt_load_store_ops >> to track all those down (not sure if the function is a good fit here). > > Hmm. Looking at what the pass is doing in more detail, I'm not sure > this approach to handling m_clobbers is safe. The pass walks the > blocks in sequence (rather than using a dom walk, say): > > FOR_EACH_BB_FN (bb, fun) > check_block (bb); > > so it could see the clobber after a later dominating assignment. > Similarly check_call_dangling could see a use that is “protected” > by a later assignment. check_call_dangling() reports only uses that are dominated by prior clobbers (determined in use_after_inval_p) so it should not have this problem. Martin > > Richard
Martin Sebor <msebor@gmail.com> writes: > On 1/19/22 03:09, Richard Sandiford wrote: >> Richard Biener <richard.guenther@gmail.com> writes: >>> On Tue, Jan 18, 2022 at 2:40 PM Richard Sandiford via Gcc-patches >>> <gcc-patches@gcc.gnu.org> wrote: >>>> >>>> In this PR the waccess pass was fed: >>>> >>>> D.10779 ={v} {CLOBBER}; >>>> VIEW_CONVERT_EXPR<svuint64_t[2]>(D.10779) = .MASK_LOAD_LANES (addr_5(D), 64B, _2); >>>> _7 = D.10779.__val[0]; >>>> >>>> However, the tracking of m_clobbers only looked at gassigns, >>>> so it missed that the clobber on the first line was overwritten >>>> by the call on the second line. >>> >>> Just as a note another possible def can come via asm() outputs >>> and clobbers. There would have been walk_stmt_load_store_ops >>> to track all those down (not sure if the function is a good fit here). >> >> Hmm. Looking at what the pass is doing in more detail, I'm not sure >> this approach to handling m_clobbers is safe. The pass walks the >> blocks in sequence (rather than using a dom walk, say): >> >> FOR_EACH_BB_FN (bb, fun) >> check_block (bb); >> >> so it could see the clobber after a later dominating assignment. >> Similarly check_call_dangling could see a use that is “protected” >> by a later assignment. > > check_call_dangling() reports only uses that are dominated by prior > clobbers (determined in use_after_inval_p) so it should not have > this problem. Yeah, but what I mean is that, if we have: A dominates B dominates C A clobbers X B defines X C uses X we could still see them in this order: A, C, B The dominance check would then succeed for <A, C> even though B should invalidate the clobber. Thanks, Richard
On 1/19/22 09:22, Richard Sandiford wrote: > Martin Sebor <msebor@gmail.com> writes: >> On 1/19/22 03:09, Richard Sandiford wrote: >>> Richard Biener <richard.guenther@gmail.com> writes: >>>> On Tue, Jan 18, 2022 at 2:40 PM Richard Sandiford via Gcc-patches >>>> <gcc-patches@gcc.gnu.org> wrote: >>>>> >>>>> In this PR the waccess pass was fed: >>>>> >>>>> D.10779 ={v} {CLOBBER}; >>>>> VIEW_CONVERT_EXPR<svuint64_t[2]>(D.10779) = .MASK_LOAD_LANES (addr_5(D), 64B, _2); >>>>> _7 = D.10779.__val[0]; >>>>> >>>>> However, the tracking of m_clobbers only looked at gassigns, >>>>> so it missed that the clobber on the first line was overwritten >>>>> by the call on the second line. >>>> >>>> Just as a note another possible def can come via asm() outputs >>>> and clobbers. There would have been walk_stmt_load_store_ops >>>> to track all those down (not sure if the function is a good fit here). >>> >>> Hmm. Looking at what the pass is doing in more detail, I'm not sure >>> this approach to handling m_clobbers is safe. The pass walks the >>> blocks in sequence (rather than using a dom walk, say): >>> >>> FOR_EACH_BB_FN (bb, fun) >>> check_block (bb); >>> >>> so it could see the clobber after a later dominating assignment. >>> Similarly check_call_dangling could see a use that is “protected” >>> by a later assignment. >> >> check_call_dangling() reports only uses that are dominated by prior >> clobbers (determined in use_after_inval_p) so it should not have >> this problem. > > Yeah, but what I mean is that, if we have: > > A dominates B dominates C > A clobbers X > B defines X > C uses X > > we could still see them in this order: > > A, C, B > > The dominance check would then succeed for <A, C> even though B > should invalidate the clobber. I see. I think you're right, that case of "clobber revival" isn't handled. I don't know how to trigger it or have a sense of how often it might come up (the dangling check runs only very early, before loop unrolling, to try to avoid it as much as possible). But running the first loop in dominator order instead as you suggest should be easy enough. Do you happen to have an idea for a test case to trigger the problem and verify it's fixed? Martin > > Thanks, > Richard
Richard Sandiford <richard.sandiford@arm.com> writes: > In this PR the waccess pass was fed: > > D.10779 ={v} {CLOBBER}; > VIEW_CONVERT_EXPR<svuint64_t[2]>(D.10779) = .MASK_LOAD_LANES (addr_5(D), 64B, _2); > _7 = D.10779.__val[0]; > > However, the tracking of m_clobbers only looked at gassigns, > so it missed that the clobber on the first line was overwritten > by the call on the second line. > > This patch splits the updating of m_clobbers out into its own > function, called after the check_*() routines, and extends it > to handle both gassigns and gcalls. I think that makes sense > as an instance of the "read, operate, write" model, with the > new function being part of "write". > > Previously only the gimple_clobber_p handling was conditional > on m_check_dangling_p, but I think the whole of the new function > can be. We only enter stmts into m_clobbers if m_check_dangling_p, > so we only need to remove them under the same condition. > > Tested on aarch64-linux-gnu. OK to install? > > Richard > > > gcc/ > PR middle-end/104092 > * gimple-ssa-warn-access.cc (pass_waccess::update_clobbers_from_lhs): > New function, split out from... > (pass_waccess::check_stmt): ...here and generalized to calls. > (pass_waccess::check_block): Call it. > > gcc/testsuite/ > * gcc.target/aarch64/sve/acle/general/pr104092.c: New test. I've pushed the test to trunk after Richard's EOL fix (thanks). Richard > --- > gcc/gimple-ssa-warn-access.cc | 68 +++++++++++-------- > .../aarch64/sve/acle/general/pr104092.c | 7 ++ > 2 files changed, 48 insertions(+), 27 deletions(-) > create mode 100644 gcc/testsuite/gcc.target/aarch64/sve/acle/general/pr104092.c > > diff --git a/gcc/gimple-ssa-warn-access.cc b/gcc/gimple-ssa-warn-access.cc > index f639807a78a..25066fa6b89 100644 > --- a/gcc/gimple-ssa-warn-access.cc > +++ b/gcc/gimple-ssa-warn-access.cc > @@ -2094,6 +2094,9 @@ private: > /* Check a non-call statement. */ > void check_stmt (gimple *); > > + /* Update the clobber map based on the lhs of a statement. */ > + void update_clobbers_from_lhs (gimple *); > + > /* Check statements in a basic block. */ > void check_block (basic_block); > > @@ -4270,33 +4273,6 @@ is_auto_decl (tree x) > void > pass_waccess::check_stmt (gimple *stmt) > { > - if (m_check_dangling_p && gimple_clobber_p (stmt)) > - { > - /* Ignore clobber statemts in blocks with exceptional edges. */ > - basic_block bb = gimple_bb (stmt); > - edge e = EDGE_PRED (bb, 0); > - if (e->flags & EDGE_EH) > - return; > - > - tree var = gimple_assign_lhs (stmt); > - m_clobbers.put (var, stmt); > - return; > - } > - > - if (is_gimple_assign (stmt)) > - { > - /* Clobbered unnamed temporaries such as compound literals can be > - revived. Check for an assignment to one and remove it from > - M_CLOBBERS. */ > - tree lhs = gimple_assign_lhs (stmt); > - while (handled_component_p (lhs)) > - lhs = TREE_OPERAND (lhs, 0); > - > - if (is_auto_decl (lhs)) > - m_clobbers.remove (lhs); > - return; > - } > - > if (greturn *ret = dyn_cast <greturn *> (stmt)) > { > if (optimize && flag_isolate_erroneous_paths_dereference) > @@ -4326,6 +4302,42 @@ pass_waccess::check_stmt (gimple *stmt) > } > } > > +/* Update the clobber map based on the lhs of STMT. */ > + > +void > +pass_waccess::update_clobbers_from_lhs (gimple *stmt) > +{ > + if (gimple_clobber_p (stmt)) > + { > + /* Ignore clobber statements in blocks with exceptional edges. */ > + basic_block bb = gimple_bb (stmt); > + edge e = EDGE_PRED (bb, 0); > + if (e->flags & EDGE_EH) > + return; > + > + tree var = gimple_assign_lhs (stmt); > + m_clobbers.put (var, stmt); > + return; > + } > + > + if (is_gimple_assign (stmt) || is_gimple_call (stmt)) > + { > + /* Clobbered unnamed temporaries such as compound literals can be > + revived. Check for an assignment to one and remove it from > + M_CLOBBERS. */ > + tree lhs = gimple_get_lhs (stmt); > + if (!lhs) > + return; > + > + while (handled_component_p (lhs)) > + lhs = TREE_OPERAND (lhs, 0); > + > + if (is_auto_decl (lhs)) > + m_clobbers.remove (lhs); > + return; > + } > +} > + > /* Check basic block BB for invalid accesses. */ > > void > @@ -4340,6 +4352,8 @@ pass_waccess::check_block (basic_block bb) > check_call (call); > else > check_stmt (stmt); > + if (m_check_dangling_p) > + update_clobbers_from_lhs (stmt); > } > } > > diff --git a/gcc/testsuite/gcc.target/aarch64/sve/acle/general/pr104092.c b/gcc/testsuite/gcc.target/aarch64/sve/acle/general/pr104092.c > new file mode 100644 > index 00000000000..c17ece7d82f > --- /dev/null > +++ b/gcc/testsuite/gcc.target/aarch64/sve/acle/general/pr104092.c > @@ -0,0 +1,7 @@ > +/* { dg-options "-O2 -Wall" } */ > + > +#include <arm_sve.h> > + > +svuint64_t bar(svbool_t pg, const uint64_t *addr) { > + return svget2(svld2_u64(pg, addr), 0); > +}
diff --git a/gcc/gimple-ssa-warn-access.cc b/gcc/gimple-ssa-warn-access.cc index f639807a78a..25066fa6b89 100644 --- a/gcc/gimple-ssa-warn-access.cc +++ b/gcc/gimple-ssa-warn-access.cc @@ -2094,6 +2094,9 @@ private: /* Check a non-call statement. */ void check_stmt (gimple *); + /* Update the clobber map based on the lhs of a statement. */ + void update_clobbers_from_lhs (gimple *); + /* Check statements in a basic block. */ void check_block (basic_block); @@ -4270,33 +4273,6 @@ is_auto_decl (tree x) void pass_waccess::check_stmt (gimple *stmt) { - if (m_check_dangling_p && gimple_clobber_p (stmt)) - { - /* Ignore clobber statemts in blocks with exceptional edges. */ - basic_block bb = gimple_bb (stmt); - edge e = EDGE_PRED (bb, 0); - if (e->flags & EDGE_EH) - return; - - tree var = gimple_assign_lhs (stmt); - m_clobbers.put (var, stmt); - return; - } - - if (is_gimple_assign (stmt)) - { - /* Clobbered unnamed temporaries such as compound literals can be - revived. Check for an assignment to one and remove it from - M_CLOBBERS. */ - tree lhs = gimple_assign_lhs (stmt); - while (handled_component_p (lhs)) - lhs = TREE_OPERAND (lhs, 0); - - if (is_auto_decl (lhs)) - m_clobbers.remove (lhs); - return; - } - if (greturn *ret = dyn_cast <greturn *> (stmt)) { if (optimize && flag_isolate_erroneous_paths_dereference) @@ -4326,6 +4302,42 @@ pass_waccess::check_stmt (gimple *stmt) } } +/* Update the clobber map based on the lhs of STMT. */ + +void +pass_waccess::update_clobbers_from_lhs (gimple *stmt) +{ + if (gimple_clobber_p (stmt)) + { + /* Ignore clobber statements in blocks with exceptional edges. */ + basic_block bb = gimple_bb (stmt); + edge e = EDGE_PRED (bb, 0); + if (e->flags & EDGE_EH) + return; + + tree var = gimple_assign_lhs (stmt); + m_clobbers.put (var, stmt); + return; + } + + if (is_gimple_assign (stmt) || is_gimple_call (stmt)) + { + /* Clobbered unnamed temporaries such as compound literals can be + revived. Check for an assignment to one and remove it from + M_CLOBBERS. */ + tree lhs = gimple_get_lhs (stmt); + if (!lhs) + return; + + while (handled_component_p (lhs)) + lhs = TREE_OPERAND (lhs, 0); + + if (is_auto_decl (lhs)) + m_clobbers.remove (lhs); + return; + } +} + /* Check basic block BB for invalid accesses. */ void @@ -4340,6 +4352,8 @@ pass_waccess::check_block (basic_block bb) check_call (call); else check_stmt (stmt); + if (m_check_dangling_p) + update_clobbers_from_lhs (stmt); } } diff --git a/gcc/testsuite/gcc.target/aarch64/sve/acle/general/pr104092.c b/gcc/testsuite/gcc.target/aarch64/sve/acle/general/pr104092.c new file mode 100644 index 00000000000..c17ece7d82f --- /dev/null +++ b/gcc/testsuite/gcc.target/aarch64/sve/acle/general/pr104092.c @@ -0,0 +1,7 @@ +/* { dg-options "-O2 -Wall" } */ + +#include <arm_sve.h> + +svuint64_t bar(svbool_t pg, const uint64_t *addr) { + return svget2(svld2_u64(pg, addr), 0); +}