On Thu, Jun 24, 2021 at 8:17 AM Jeff Law via Gcc-patches
<gcc-patches@gcc.gnu.org> wrote:
>
>
>
> On 6/23/2021 4:19 PM, apinski--- via Gcc-patches wrote:
> > From: Andrew Pinski <apin...@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.
> > Placing this in replace_phi_edge_with_variable is the best option instead
> > of doing it in each time replace_phi_edge_with_variable is called which is
> > what is done today.
> >
> > OK? Bootstrapped and tested on x86_64-linux-gnu with no regressions.
> >
> > gcc/ChangeLog:
> >
> > * tree-ssa-phiopt.c (replace_phi_edge_with_variable): Duplicate range
> > info if we're the only things setting the target PHI.
> > (value_replacement): Don't duplicate range here.
> > (minmax_replacement): Likewise.
> > ---
> > gcc/tree-ssa-phiopt.c | 35 ++++++++++++++++++-----------------
> > 1 file changed, 18 insertions(+), 17 deletions(-)
> >
> > diff --git a/gcc/tree-ssa-phiopt.c b/gcc/tree-ssa-phiopt.c
> > index 24cbce9955a..147397ad82c 100644
> > --- a/gcc/tree-ssa-phiopt.c
> > +++ b/gcc/tree-ssa-phiopt.c
> > @@ -391,6 +391,24 @@ replace_phi_edge_with_variable (basic_block cond_block,
> > basic_block bb = gimple_bb (phi);
> > basic_block block_to_remove;
> > gimple_stmt_iterator gsi;
> > + tree phi_result = PHI_RESULT (phi);
> > +
> > + /* Duplicate range info as needed. */
> > + if (TREE_CODE (new_tree) == SSA_NAME
> > + && EDGE_COUNT (gimple_bb (phi)->preds) == 2
> > + && INTEGRAL_TYPE_P (TREE_TYPE (phi_result)))
> > + {
> > + if (!SSA_NAME_RANGE_INFO (new_tree)
> > + && SSA_NAME_RANGE_INFO (phi_result))
> > + duplicate_ssa_name_range_info (new_tree,
> > + SSA_NAME_RANGE_TYPE (phi_result),
> > + SSA_NAME_RANGE_INFO (phi_result));
> > + if (SSA_NAME_RANGE_INFO (new_tree)
> > + && !SSA_NAME_RANGE_INFO (phi_result))
> > + duplicate_ssa_name_range_info (phi_result,
> > + SSA_NAME_RANGE_TYPE (new_tree),
> > + SSA_NAME_RANGE_INFO (new_tree));
> > + }
> I'm not sure you need the EDGE_COUNT test here.
I think we need it. See below for the reason why.
>
> I worry that copying the range info from the argument to the PHI result
> isn't right. It was OK for the ABS replacement, but I'm not sure that's
> true in general.
Right, I had originally placed the code that was done in
minmax_replacement in match_simplify_replacement.
But Richi wanted me to move the code to replace_phi_edge_with_variable
and have it be able to go both directions for the range.
The reason why the check for the number of incoming edges is needed is
because if we start with:
Range[xyz]
c_1 = PHI <a_2(3), b_3(4), c_4(5)>
And we are replacing the variable for edges 4 and 5 only, then we
would get the wrong range on the new SSA name as it would include the
range of a_2 which is not correct.
Also we would not lose the range information in that case either.
This is more about losing the range information.
In the case where this shows up is we have:
bb1:
if (a_1 >= 255) goto bb2; else goto bb3;
bb2:
a_2 = 255;
goto bb3;
range<-INF,255>
a_3 = PHI<a_1(1), a_2(2)>
Which gets translated into
bb1:
_4 = min<a_1, 255>
goto bb2
range<-INF,255>
a_3 = PHI<_4(1)>
bb3:
use(a_3)
And when cfg-cleanup merges the two basic blocks, _4 is propagated
into where a_3 was used and the range information is now gone.
So the idea is to propagate the range info from a_3 back on to _4 if
we know that they propagated later on.
The opposite way (range of _4 on to a_3) actually does not really
matter as we are going to throw away the PHI node anyways.
Thanks,
Andrew Pinski
>
> Jeff
>
>
> >
> > /* Change the PHI argument to new. */
> > SET_USE (PHI_ARG_DEF_PTR (phi, e->dest_idx), new_tree);
> > @@ -1385,16 +1403,6 @@ value_replacement (basic_block cond_bb, basic_block
> > middle_bb,
> > <bb 4>:
> > # u_3 = PHI <u_6(3), 4294967295(2)> */
> > reset_flow_sensitive_info (lhs);
> > - if (INTEGRAL_TYPE_P (TREE_TYPE (lhs)))
> > - {
> > - /* If available, we can use VR of phi result at least. */
> > - tree phires = gimple_phi_result (phi);
> > - struct range_info_def *phires_range_info
> > - = SSA_NAME_RANGE_INFO (phires);
> > - if (phires_range_info)
> > - duplicate_ssa_name_range_info (lhs, SSA_NAME_RANGE_TYPE (phires),
> > - phires_range_info);
> > - }
> > gimple_stmt_iterator gsi_from;
> > for (int i = prep_cnt - 1; i >= 0; --i)
> > {
> > @@ -1794,13 +1802,6 @@ minmax_replacement (basic_block cond_bb, basic_block
> > middle_bb,
> > gimple_seq stmts = NULL;
> > tree phi_result = PHI_RESULT (phi);
> > result = gimple_build (&stmts, minmax, TREE_TYPE (phi_result), arg0,
> > arg1);
> > - /* Duplicate range info if we're the only things setting the target PHI.
> > */
> > - if (!gimple_seq_empty_p (stmts)
> > - && 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));
> >
> > gsi = gsi_last_bb (cond_bb);
> > gsi_insert_seq_before (&gsi, stmts, GSI_NEW_STMT);
>
