* [PATCH 1/7] Reset the range info on the moved instruction in PHIOPT
@ 2021-06-19 19:47 apinski
2021-06-19 19:47 ` [PATCH 2/7] Duplicate the range information of the phi onto the new ssa_name apinski
2021-06-21 6:42 ` [PATCH 1/7] Reset the range info on the moved instruction in PHIOPT Richard Biener
0 siblings, 2 replies; 6+ messages in thread
From: apinski @ 2021-06-19 19:47 UTC (permalink / raw)
To: gcc-patches; +Cc: Andrew Pinski
From: Andrew Pinski <apinski@marvell.com>
I had missed this when wrote the patch which allowed the
gimple to be moved from inside the conditional as it. It
was also missed in the review. Anyways the range information
needs to be reset for the moved gimple as it was under a
conditional and the flow has changed to be unconditional.
I have not seen any testcase in the wild that produces wrong code
yet which is why there is no testcase but this is similar to what
the other code in phiopt does so after moving those to match, there
might be some.
OK? Bootstrapped and tested on x86_64-linux-gnu with no regressions.
gcc/ChangeLog:
* tree-ssa-phiopt.c (match_simplify_replacement): Reset
flow senatitive info on the moved ssa set.
---
gcc/tree-ssa-phiopt.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/gcc/tree-ssa-phiopt.c b/gcc/tree-ssa-phiopt.c
index 02e26f974a5..24cbce9955a 100644
--- a/gcc/tree-ssa-phiopt.c
+++ b/gcc/tree-ssa-phiopt.c
@@ -836,7 +836,7 @@ match_simplify_replacement (basic_block cond_bb, basic_block middle_bb,
if (!is_gimple_assign (stmt_to_move))
return false;
- tree lhs = gimple_assign_lhs (stmt_to_move);
+ tree lhs = gimple_assign_lhs (stmt_to_move);
gimple *use_stmt;
use_operand_p use_p;
@@ -892,6 +892,7 @@ match_simplify_replacement (basic_block cond_bb, basic_block middle_bb,
}
gimple_stmt_iterator gsi1 = gsi_for_stmt (stmt_to_move);
gsi_move_before (&gsi1, &gsi);
+ reset_flow_sensitive_info (gimple_assign_lhs (stmt_to_move));
}
if (seq)
gsi_insert_seq_before (&gsi, seq, GSI_SAME_STMT);
--
2.27.0
^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH 2/7] Duplicate the range information of the phi onto the new ssa_name
2021-06-19 19:47 [PATCH 1/7] Reset the range info on the moved instruction in PHIOPT apinski
@ 2021-06-19 19:47 ` apinski
2021-06-21 6:49 ` Richard Biener
2021-06-21 6:42 ` [PATCH 1/7] Reset the range info on the moved instruction in PHIOPT Richard Biener
1 sibling, 1 reply; 6+ messages in thread
From: apinski @ 2021-06-19 19:47 UTC (permalink / raw)
To: gcc-patches; +Cc: Andrew Pinski
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))
+ && 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);
--
2.27.0
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 1/7] Reset the range info on the moved instruction in PHIOPT
2021-06-19 19:47 [PATCH 1/7] Reset the range info on the moved instruction in PHIOPT apinski
2021-06-19 19:47 ` [PATCH 2/7] Duplicate the range information of the phi onto the new ssa_name apinski
@ 2021-06-21 6:42 ` Richard Biener
1 sibling, 0 replies; 6+ messages in thread
From: Richard Biener @ 2021-06-21 6:42 UTC (permalink / raw)
To: Andrew Pinski; +Cc: GCC Patches
On Sat, Jun 19, 2021 at 9:48 PM apinski--- via Gcc-patches
<gcc-patches@gcc.gnu.org> wrote:
>
> From: Andrew Pinski <apinski@marvell.com>
>
> I had missed this when wrote the patch which allowed the
> gimple to be moved from inside the conditional as it. It
> was also missed in the review. Anyways the range information
> needs to be reset for the moved gimple as it was under a
> conditional and the flow has changed to be unconditional.
> I have not seen any testcase in the wild that produces wrong code
> yet which is why there is no testcase but this is similar to what
> the other code in phiopt does so after moving those to match, there
> might be some.
>
> OK? Bootstrapped and tested on x86_64-linux-gnu with no regressions.
OK.
Richard.
> gcc/ChangeLog:
>
> * tree-ssa-phiopt.c (match_simplify_replacement): Reset
> flow senatitive info on the moved ssa set.
> ---
> gcc/tree-ssa-phiopt.c | 3 ++-
> 1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/gcc/tree-ssa-phiopt.c b/gcc/tree-ssa-phiopt.c
> index 02e26f974a5..24cbce9955a 100644
> --- a/gcc/tree-ssa-phiopt.c
> +++ b/gcc/tree-ssa-phiopt.c
> @@ -836,7 +836,7 @@ match_simplify_replacement (basic_block cond_bb, basic_block middle_bb,
> if (!is_gimple_assign (stmt_to_move))
> return false;
>
> - tree lhs = gimple_assign_lhs (stmt_to_move);
> + tree lhs = gimple_assign_lhs (stmt_to_move);
> gimple *use_stmt;
> use_operand_p use_p;
>
> @@ -892,6 +892,7 @@ match_simplify_replacement (basic_block cond_bb, basic_block middle_bb,
> }
> gimple_stmt_iterator gsi1 = gsi_for_stmt (stmt_to_move);
> gsi_move_before (&gsi1, &gsi);
> + reset_flow_sensitive_info (gimple_assign_lhs (stmt_to_move));
> }
> if (seq)
> gsi_insert_seq_before (&gsi, seq, GSI_SAME_STMT);
> --
> 2.27.0
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 2/7] Duplicate the range information of the phi onto the new ssa_name
2021-06-19 19:47 ` [PATCH 2/7] Duplicate the range information of the phi onto the new ssa_name apinski
@ 2021-06-21 6:49 ` Richard Biener
2021-06-23 0:19 ` Andrew Pinski
0 siblings, 1 reply; 6+ messages in thread
From: Richard Biener @ 2021-06-21 6:49 UTC (permalink / raw)
To: Andrew Pinski; +Cc: GCC Patches
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
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 2/7] Duplicate the range information of the phi onto the new ssa_name
2021-06-21 6:49 ` Richard Biener
@ 2021-06-23 0:19 ` Andrew Pinski
2021-06-23 7:29 ` Richard Biener
0 siblings, 1 reply; 6+ messages in thread
From: Andrew Pinski @ 2021-06-23 0:19 UTC (permalink / raw)
To: Richard Biener; +Cc: Andrew Pinski, GCC Patches
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
> >
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 2/7] Duplicate the range information of the phi onto the new ssa_name
2021-06-23 0:19 ` Andrew Pinski
@ 2021-06-23 7:29 ` Richard Biener
0 siblings, 0 replies; 6+ messages in thread
From: Richard Biener @ 2021-06-23 7:29 UTC (permalink / raw)
To: Andrew Pinski; +Cc: Andrew Pinski, GCC Patches
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
> > >
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2021-06-23 7:29 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-06-19 19:47 [PATCH 1/7] Reset the range info on the moved instruction in PHIOPT apinski
2021-06-19 19:47 ` [PATCH 2/7] Duplicate the range information of the phi onto the new ssa_name apinski
2021-06-21 6:49 ` Richard Biener
2021-06-23 0:19 ` Andrew Pinski
2021-06-23 7:29 ` Richard Biener
2021-06-21 6:42 ` [PATCH 1/7] Reset the range info on the moved instruction in PHIOPT Richard Biener
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).