waccess: Look at calls when tracking clobbers [PR104092]

  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
  

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);
> +}