| Message ID | 20240515114105.0AA7F1372E@imap1.dmz-prg2.suse.org |
|---|---|
| State | New |
| Headers | show |
| Series | middle-end/111422 - wrong stack var coalescing, handle PHIs | expand |
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 <gassign *> (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 <gphi *> (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 <gassign *> (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
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 <gassign *> (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 <gphi *> (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 <gassign *> (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.
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 <gassign *> (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 <gphi *> (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 <gassign *> (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); + } + } } }
The gcc.c-torture/execute/pr111422.c testcase after installing the sink pass improvement reveals that we also need to handle _65 = &g + _58; _44 = &g + _43; # _59 = PHI <_65, _44> *_59 = 8; g = {v} {CLOBBER(eos)}; ... n[0] = &f; *_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(-)