* Re: [PATCH] waccess: Look at calls when tracking clobbers [PR104092]
2022-01-18 13:37 [PATCH] waccess: Look at calls when tracking clobbers [PR104092] Richard Sandiford
@ 2022-01-18 18:06 ` Martin Sebor
2022-01-18 20:22 ` Jason Merrill
2022-01-19 9:22 ` Richard Biener
2022-02-04 8:10 ` Richard Sandiford
2 siblings, 1 reply; 9+ messages in thread
From: Martin Sebor @ 2022-01-18 18:06 UTC (permalink / raw)
To: gcc-patches, msebor, richard.sandiford
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);
> +}
^ permalink raw reply [flat|nested] 9+ messages in thread* Re: [PATCH] waccess: Look at calls when tracking clobbers [PR104092]
2022-01-18 18:06 ` Martin Sebor
@ 2022-01-18 20:22 ` Jason Merrill
0 siblings, 0 replies; 9+ messages in thread
From: Jason Merrill @ 2022-01-18 20:22 UTC (permalink / raw)
To: Martin Sebor, gcc-patches, msebor, richard.sandiford
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);
>> +}
>
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] waccess: Look at calls when tracking clobbers [PR104092]
2022-01-18 13:37 [PATCH] waccess: Look at calls when tracking clobbers [PR104092] Richard Sandiford
2022-01-18 18:06 ` Martin Sebor
@ 2022-01-19 9:22 ` Richard Biener
2022-01-19 10:09 ` Richard Sandiford
2022-02-04 8:10 ` Richard Sandiford
2 siblings, 1 reply; 9+ messages in thread
From: Richard Biener @ 2022-01-19 9:22 UTC (permalink / raw)
To: Richard Sandiford, GCC Patches, msebor
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
>
^ permalink raw reply [flat|nested] 9+ messages in thread* Re: [PATCH] waccess: Look at calls when tracking clobbers [PR104092]
2022-01-19 9:22 ` Richard Biener
@ 2022-01-19 10:09 ` Richard Sandiford
2022-01-19 16:12 ` Martin Sebor
0 siblings, 1 reply; 9+ messages in thread
From: Richard Sandiford @ 2022-01-19 10:09 UTC (permalink / raw)
To: Richard Biener; +Cc: GCC Patches, msebor
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
^ permalink raw reply [flat|nested] 9+ messages in thread* Re: [PATCH] waccess: Look at calls when tracking clobbers [PR104092]
2022-01-19 10:09 ` Richard Sandiford
@ 2022-01-19 16:12 ` Martin Sebor
2022-01-19 16:22 ` Richard Sandiford
0 siblings, 1 reply; 9+ messages in thread
From: Martin Sebor @ 2022-01-19 16:12 UTC (permalink / raw)
To: Richard Biener, GCC Patches, msebor, richard.sandiford
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
^ permalink raw reply [flat|nested] 9+ messages in thread* Re: [PATCH] waccess: Look at calls when tracking clobbers [PR104092]
2022-01-19 16:12 ` Martin Sebor
@ 2022-01-19 16:22 ` Richard Sandiford
2022-01-19 17:12 ` Martin Sebor
0 siblings, 1 reply; 9+ messages in thread
From: Richard Sandiford @ 2022-01-19 16:22 UTC (permalink / raw)
To: Martin Sebor; +Cc: Richard Biener, GCC Patches, msebor
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
^ permalink raw reply [flat|nested] 9+ messages in thread* Re: [PATCH] waccess: Look at calls when tracking clobbers [PR104092]
2022-01-19 16:22 ` Richard Sandiford
@ 2022-01-19 17:12 ` Martin Sebor
0 siblings, 0 replies; 9+ messages in thread
From: Martin Sebor @ 2022-01-19 17:12 UTC (permalink / raw)
To: Richard Biener, GCC Patches, msebor, richard.sandiford
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
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] waccess: Look at calls when tracking clobbers [PR104092]
2022-01-18 13:37 [PATCH] waccess: Look at calls when tracking clobbers [PR104092] Richard Sandiford
2022-01-18 18:06 ` Martin Sebor
2022-01-19 9:22 ` Richard Biener
@ 2022-02-04 8:10 ` Richard Sandiford
2 siblings, 0 replies; 9+ messages in thread
From: Richard Sandiford @ 2022-02-04 8:10 UTC (permalink / raw)
To: gcc-patches; +Cc: msebor
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);
> +}
^ permalink raw reply [flat|nested] 9+ messages in thread