| Message ID | 1624132041-665-2-git-send-email-apinski@marvell.com |
|---|---|
| State | New |
| Headers | show |
| Series | [1/7] Reset the range info on the moved instruction in PHIOPT | expand |
On Sat, Jun 19, 2021 at 9:49 PM apinski--- via Gcc-patches <gcc-patches@gcc.gnu.org> wrote: > > From: Andrew Pinski <apinski@marvell.com> > > Since match_simplify_replacement uses gimple_simplify, there is a new > ssa name created sometimes and then we go and replace the phi edge with > this new ssa name, the range information on the phi is lost. > I don't have a testcase right now where we lose the range information > though but it does show up when enhancing match.pd to handle > some min/max patterns and g++.dg/warn/Wstringop-overflow-1.C starts > to fail. > > OK? Bootstrapped and tested on x86_64-linux-gnu with no regressions. > > gcc/ChangeLog: > > * tree-ssa-phiopt.c (match_simplify_replacement): Duplicate range > info if we're the only things setting the target PHI. > --- > gcc/tree-ssa-phiopt.c | 8 ++++++++ > 1 file changed, 8 insertions(+) > > diff --git a/gcc/tree-ssa-phiopt.c b/gcc/tree-ssa-phiopt.c > index 24cbce9955a..feb8ca8d0d1 100644 > --- a/gcc/tree-ssa-phiopt.c > +++ b/gcc/tree-ssa-phiopt.c > @@ -894,6 +894,14 @@ match_simplify_replacement (basic_block cond_bb, basic_block middle_bb, > gsi_move_before (&gsi1, &gsi); > reset_flow_sensitive_info (gimple_assign_lhs (stmt_to_move)); > } > + /* Duplicate range info if we're the only things setting the target PHI. */ > + tree phi_result = PHI_RESULT (phi); > + if (!gimple_seq_empty_p (seq) > + && EDGE_COUNT (gimple_bb (phi)->preds) == 2 > + && !POINTER_TYPE_P (TREE_TYPE (phi_result)) Please use INTEGRAL_TYPE_P (...) > + && SSA_NAME_RANGE_INFO (phi_result) && !SSA_NAME_RANGE_INFO (result) ? Why conditional on !gimple_seq_empty_p (seq)? It looks like we could do this trick (actually in both directions, wherever the range info is missing?) in replace_phi_edge_with_variable instead? Thanks, Richard. ) > + duplicate_ssa_name_range_info (result, SSA_NAME_RANGE_TYPE (phi_result), > + SSA_NAME_RANGE_INFO (phi_result)); > if (seq) > gsi_insert_seq_before (&gsi, seq, GSI_SAME_STMT); > > -- > 2.27.0 >
On Sun, Jun 20, 2021 at 11:50 PM Richard Biener via Gcc-patches <gcc-patches@gcc.gnu.org> wrote: > > On Sat, Jun 19, 2021 at 9:49 PM apinski--- via Gcc-patches > <gcc-patches@gcc.gnu.org> wrote: > > > > From: Andrew Pinski <apinski@marvell.com> > > > > Since match_simplify_replacement uses gimple_simplify, there is a new > > ssa name created sometimes and then we go and replace the phi edge with > > this new ssa name, the range information on the phi is lost. > > I don't have a testcase right now where we lose the range information > > though but it does show up when enhancing match.pd to handle > > some min/max patterns and g++.dg/warn/Wstringop-overflow-1.C starts > > to fail. > > > > OK? Bootstrapped and tested on x86_64-linux-gnu with no regressions. > > > > gcc/ChangeLog: > > > > * tree-ssa-phiopt.c (match_simplify_replacement): Duplicate range > > info if we're the only things setting the target PHI. > > --- > > gcc/tree-ssa-phiopt.c | 8 ++++++++ > > 1 file changed, 8 insertions(+) > > > > diff --git a/gcc/tree-ssa-phiopt.c b/gcc/tree-ssa-phiopt.c > > index 24cbce9955a..feb8ca8d0d1 100644 > > --- a/gcc/tree-ssa-phiopt.c > > +++ b/gcc/tree-ssa-phiopt.c > > @@ -894,6 +894,14 @@ match_simplify_replacement (basic_block cond_bb, basic_block middle_bb, > > gsi_move_before (&gsi1, &gsi); > > reset_flow_sensitive_info (gimple_assign_lhs (stmt_to_move)); > > } > > + /* Duplicate range info if we're the only things setting the target PHI. */ > > + tree phi_result = PHI_RESULT (phi); > > + if (!gimple_seq_empty_p (seq) > > + && EDGE_COUNT (gimple_bb (phi)->preds) == 2 > > + && !POINTER_TYPE_P (TREE_TYPE (phi_result)) > > Please use INTEGRAL_TYPE_P (...) Yes, using INTEGRAL_TYPE_P makes more sense here. Note I copied exactly what was already done in minmax_replacement. > > > + && SSA_NAME_RANGE_INFO (phi_result) > > && !SSA_NAME_RANGE_INFO (result) > > ? Why conditional on !gimple_seq_empty_p (seq)? The way I understand it is if !gimple_seq_empty_p (seq) is true then the result will be a new SSA name and therefore !SSA_NAME_RANGE_INFO (result) will also be true > It looks like we could do this trick (actually in both directions, > wherever the range > info is missing?) in replace_phi_edge_with_variable instead? Let me look into doing that. Thanks, Andrew > > Thanks, > Richard. > > ) > > + duplicate_ssa_name_range_info (result, SSA_NAME_RANGE_TYPE (phi_result), > > + SSA_NAME_RANGE_INFO (phi_result)); > > if (seq) > > gsi_insert_seq_before (&gsi, seq, GSI_SAME_STMT); > > > > -- > > 2.27.0 > >
On Wed, Jun 23, 2021 at 2:19 AM Andrew Pinski <pinskia@gmail.com> wrote: > > On Sun, Jun 20, 2021 at 11:50 PM Richard Biener via Gcc-patches > <gcc-patches@gcc.gnu.org> wrote: > > > > On Sat, Jun 19, 2021 at 9:49 PM apinski--- via Gcc-patches > > <gcc-patches@gcc.gnu.org> wrote: > > > > > > From: Andrew Pinski <apinski@marvell.com> > > > > > > Since match_simplify_replacement uses gimple_simplify, there is a new > > > ssa name created sometimes and then we go and replace the phi edge with > > > this new ssa name, the range information on the phi is lost. > > > I don't have a testcase right now where we lose the range information > > > though but it does show up when enhancing match.pd to handle > > > some min/max patterns and g++.dg/warn/Wstringop-overflow-1.C starts > > > to fail. > > > > > > OK? Bootstrapped and tested on x86_64-linux-gnu with no regressions. > > > > > > gcc/ChangeLog: > > > > > > * tree-ssa-phiopt.c (match_simplify_replacement): Duplicate range > > > info if we're the only things setting the target PHI. > > > --- > > > gcc/tree-ssa-phiopt.c | 8 ++++++++ > > > 1 file changed, 8 insertions(+) > > > > > > diff --git a/gcc/tree-ssa-phiopt.c b/gcc/tree-ssa-phiopt.c > > > index 24cbce9955a..feb8ca8d0d1 100644 > > > --- a/gcc/tree-ssa-phiopt.c > > > +++ b/gcc/tree-ssa-phiopt.c > > > @@ -894,6 +894,14 @@ match_simplify_replacement (basic_block cond_bb, basic_block middle_bb, > > > gsi_move_before (&gsi1, &gsi); > > > reset_flow_sensitive_info (gimple_assign_lhs (stmt_to_move)); > > > } > > > + /* Duplicate range info if we're the only things setting the target PHI. */ > > > + tree phi_result = PHI_RESULT (phi); > > > + if (!gimple_seq_empty_p (seq) > > > + && EDGE_COUNT (gimple_bb (phi)->preds) == 2 > > > + && !POINTER_TYPE_P (TREE_TYPE (phi_result)) > > > > Please use INTEGRAL_TYPE_P (...) > > Yes, using INTEGRAL_TYPE_P makes more sense here. > Note I copied exactly what was already done in minmax_replacement. > > > > > > + && SSA_NAME_RANGE_INFO (phi_result) > > > > && !SSA_NAME_RANGE_INFO (result) > > > > ? Why conditional on !gimple_seq_empty_p (seq)? > > The way I understand it is if !gimple_seq_empty_p (seq) is true then > the result will be a new SSA name and therefore !SSA_NAME_RANGE_INFO > (result) will also be true Indeed. But then when the whole thing simplifies to an existing def we can still copy the range in case it is not present on the def, or if it is present on the def, we can copy it to the PHI result. > > It looks like we could do this trick (actually in both directions, > > wherever the range > > info is missing?) in replace_phi_edge_with_variable instead? > > Let me look into doing that. Thanks. > Thanks, > Andrew > > > > > Thanks, > > Richard. > > > > ) > > > + duplicate_ssa_name_range_info (result, SSA_NAME_RANGE_TYPE (phi_result), > > > + SSA_NAME_RANGE_INFO (phi_result)); > > > if (seq) > > > gsi_insert_seq_before (&gsi, seq, GSI_SAME_STMT); > > > > > > -- > > > 2.27.0 > > >
diff --git a/gcc/tree-ssa-phiopt.c b/gcc/tree-ssa-phiopt.c index 24cbce9955a..feb8ca8d0d1 100644 --- a/gcc/tree-ssa-phiopt.c +++ b/gcc/tree-ssa-phiopt.c @@ -894,6 +894,14 @@ match_simplify_replacement (basic_block cond_bb, basic_block middle_bb, gsi_move_before (&gsi1, &gsi); reset_flow_sensitive_info (gimple_assign_lhs (stmt_to_move)); } + /* Duplicate range info if we're the only things setting the target PHI. */ + tree phi_result = PHI_RESULT (phi); + if (!gimple_seq_empty_p (seq) + && EDGE_COUNT (gimple_bb (phi)->preds) == 2 + && !POINTER_TYPE_P (TREE_TYPE (phi_result)) + && SSA_NAME_RANGE_INFO (phi_result)) + duplicate_ssa_name_range_info (result, SSA_NAME_RANGE_TYPE (phi_result), + SSA_NAME_RANGE_INFO (phi_result)); if (seq) gsi_insert_seq_before (&gsi, seq, GSI_SAME_STMT);
From: Andrew Pinski <apinski@marvell.com> Since match_simplify_replacement uses gimple_simplify, there is a new ssa name created sometimes and then we go and replace the phi edge with this new ssa name, the range information on the phi is lost. I don't have a testcase right now where we lose the range information though but it does show up when enhancing match.pd to handle some min/max patterns and g++.dg/warn/Wstringop-overflow-1.C starts to fail. OK? Bootstrapped and tested on x86_64-linux-gnu with no regressions. gcc/ChangeLog: * tree-ssa-phiopt.c (match_simplify_replacement): Duplicate range info if we're the only things setting the target PHI. --- gcc/tree-ssa-phiopt.c | 8 ++++++++ 1 file changed, 8 insertions(+)