Re: [PATCH] middle-end/111422 - wrong stack var coalescing, handle PHIs

[Richard Biener]

On Wed, 15 May 2024, Jakub Jelinek wrote:

> On Wed, May 15, 2024 at 01:41:04PM +0200, Richard Biener wrote:
> > PR middle-end/111422
> > * cfgexpand.cc (add_scope_conflicts_2): Handle PHIs
> > by recursing to their arguments.
> > ---
> >  gcc/cfgexpand.cc | 21 +
> >  1 file changed, 17 insertions(+), 4 deletions(-)
> > 
> > diff --git a/gcc/cfgexpand.cc b/gcc/cfgexpand.cc
> > index 557cb28733b..e4d763fa998 100644
> > --- a/gcc/cfgexpand.cc
> > +++ b/gcc/cfgexpand.cc
> > @@ -584,10 +584,23 @@ add_scope_conflicts_2 (tree use, bitmap work,
> >   || INTEGRAL_TYPE_P (TREE_TYPE (use
> >  {
> >gimple *g = SSA_NAME_DEF_STMT (use);
> > -  if (is_gimple_assign (g))
> > -   if (tree op = gimple_assign_rhs1 (g))
> > - if (TREE_CODE (op) == ADDR_EXPR)
> > -   visit (g, TREE_OPERAND (op, 0), op, work);
> > +  if (gassign *a = dyn_cast  (g))
> > +   {
> > + if (tree op = gimple_assign_rhs1 (a))
> > +   if (TREE_CODE (op) == ADDR_EXPR)
> > + visit (a, TREE_OPERAND (op, 0), op, work);
> > +   }
> > +  else if (gphi *p = dyn_cast  (g))
> > +   {
> > + for (unsigned i = 0; i < gimple_phi_num_args (p); ++i)
> > +   if (TREE_CODE (use = gimple_phi_arg_def (p, i)) == SSA_NAME)
> > + if (gassign *a = dyn_cast  (SSA_NAME_DEF_STMT (use)))
> > +   {
> > + if (tree op = gimple_assign_rhs1 (a))
> > +   if (TREE_CODE (op) == ADDR_EXPR)
> > + visit (a, TREE_OPERAND (op, 0), op, work);
> > +   }
> > +   }
> 
> Why the 2 {} pairs here?  Can't it be done without them (sure, before the
> else if it is required)?

Removed and pushed.

Richard.

Re: [PATCH] middle-end/111422 - wrong stack var coalescing, handle PHIs

[Jakub Jelinek]

On Wed, May 15, 2024 at 01:41:04PM +0200, Richard Biener wrote:
>   PR middle-end/111422
>   * cfgexpand.cc (add_scope_conflicts_2): Handle PHIs
>   by recursing to their arguments.
> ---
>  gcc/cfgexpand.cc | 21 +
>  1 file changed, 17 insertions(+), 4 deletions(-)
> 
> diff --git a/gcc/cfgexpand.cc b/gcc/cfgexpand.cc
> index 557cb28733b..e4d763fa998 100644
> --- a/gcc/cfgexpand.cc
> +++ b/gcc/cfgexpand.cc
> @@ -584,10 +584,23 @@ add_scope_conflicts_2 (tree use, bitmap work,
> || INTEGRAL_TYPE_P (TREE_TYPE (use
>  {
>gimple *g = SSA_NAME_DEF_STMT (use);
> -  if (is_gimple_assign (g))
> - if (tree op = gimple_assign_rhs1 (g))
> -   if (TREE_CODE (op) == ADDR_EXPR)
> - visit (g, TREE_OPERAND (op, 0), op, work);
> +  if (gassign *a = dyn_cast  (g))
> + {
> +   if (tree op = gimple_assign_rhs1 (a))
> + if (TREE_CODE (op) == ADDR_EXPR)
> +   visit (a, TREE_OPERAND (op, 0), op, work);
> + }
> +  else if (gphi *p = dyn_cast  (g))
> + {
> +   for (unsigned i = 0; i < gimple_phi_num_args (p); ++i)
> + if (TREE_CODE (use = gimple_phi_arg_def (p, i)) == SSA_NAME)
> +   if (gassign *a = dyn_cast  (SSA_NAME_DEF_STMT (use)))
> + {
> +   if (tree op = gimple_assign_rhs1 (a))
> + if (TREE_CODE (op) == ADDR_EXPR)
> +   visit (a, TREE_OPERAND (op, 0), op, work);
> + }
> + }

Why the 2 {} pairs here?  Can't it be done without them (sure, before the
else if it is required)?

Otherwise LGTM.

Jakub

[PATCH] middle-end/111422 - wrong stack var coalescing, handle PHIs

[Richard Biener]

The gcc.c-torture/execute/pr111422.c testcase after installing the
sink pass improvement reveals that we also need to handle

 _65 =  + _58;  _44 =  + _43;
 # _59 = PHI <_65, _44>
 *_59 = 8;
 g = {v} {CLOBBER(eos)};
 ...
 n[0] = 
 *_59 = 8;
 g = {v} {CLOBBER(eos)};

where we fail to see the conflict between n and g after the first
clobber of g.  Before the sinking improvement there was a conflict
recorded on a path where _65/_44 are unused, so the real conflict
was missed but the fake one avoided the miscompile.

The following handles PHI defs in add_scope_conflicts_2 which
fixes the issue.

Bootstrapped on x86_64-unknown-linux-gnu, testing in progress.

OK if that succeeds?

Thanks,
Richard.

PR middle-end/111422
* cfgexpand.cc (add_scope_conflicts_2): Handle PHIs
by recursing to their arguments.
---
 gcc/cfgexpand.cc | 21 +
 1 file changed, 17 insertions(+), 4 deletions(-)

diff --git a/gcc/cfgexpand.cc b/gcc/cfgexpand.cc
index 557cb28733b..e4d763fa998 100644
--- a/gcc/cfgexpand.cc
+++ b/gcc/cfgexpand.cc
@@ -584,10 +584,23 @@ add_scope_conflicts_2 (tree use, bitmap work,
  || INTEGRAL_TYPE_P (TREE_TYPE (use
 {
   gimple *g = SSA_NAME_DEF_STMT (use);
-  if (is_gimple_assign (g))
-   if (tree op = gimple_assign_rhs1 (g))
- if (TREE_CODE (op) == ADDR_EXPR)
-   visit (g, TREE_OPERAND (op, 0), op, work);
+  if (gassign *a = dyn_cast  (g))
+   {
+ if (tree op = gimple_assign_rhs1 (a))
+   if (TREE_CODE (op) == ADDR_EXPR)
+ visit (a, TREE_OPERAND (op, 0), op, work);
+   }
+  else if (gphi *p = dyn_cast  (g))
+   {
+ for (unsigned i = 0; i < gimple_phi_num_args (p); ++i)
+   if (TREE_CODE (use = gimple_phi_arg_def (p, i)) == SSA_NAME)
+ if (gassign *a = dyn_cast  (SSA_NAME_DEF_STMT (use)))
+   {
+ if (tree op = gimple_assign_rhs1 (a))
+   if (TREE_CODE (op) == ADDR_EXPR)
+ visit (a, TREE_OPERAND (op, 0), op, work);
+   }
+   }
 }
 }
 
-- 
2.35.3