Re: [PATCH 1/5] Fix 101256: Wrong code due to range incorrect from PHI-OPT

[Andrew Pinski <pinskia@gmail.com>]

On Mon, Jul 5, 2021 at 4:26 AM Richard Biener via Gcc-patches
<gcc-patches@gcc.gnu.org> wrote:
>
> On Sun, Jul 4, 2021 at 8:40 PM apinski--- via Gcc-patches
> <gcc-patches@gcc.gnu.org> wrote:
> >
> > From: Andrew Pinski <apinski@marvell.com>
> >
> > So the problem here is that replace_phi_edge_with_variable
> > will copy range information to a already (not newly) defined
> > ssa name.  This causes wrong code later on.
>
> That's a bit too conservative I guess?  Shouldn't it work for at least
> all defs defined in the same block as the original conditional (and
> thus then applying to the seq inserted there by the callers)?

Yes that even simplifies the change even further and still provide the
needed ranges
I should have a patch to submit later today.

Thanks,
Andrew Pinski

>
> I realize it's wrong for, say
>
>   _1 = ..
>  if (_1 != 0)
>    {
>      ...
>     if (..)
>        ;
>      # _2 = PHI <_1, 1>
> ...
>    }
>
> with _2 having range [1, +INF] but clearly not _1 at the point of its
> definition.
>
> Richard.
>
> > This patch fixes the problem by requiring there to be statements
> > that are to be placed before the conditional to be able to
> > copy the range info; this assumes the statements will define
> > the ssa name.
> >
> > gcc/ChangeLog:
> >
> >         PR tree-optimization/101256
> >         * dbgcnt.def (phiopt_edge_range): New counter.
> >         * tree-ssa-phiopt.c (replace_phi_edge_with_variable):
> >         Add optional sequence which will be added before the old
> >         conditional. Check sequence for non-null if we want to
> >         update the range.
> >         (two_value_replacement): Instead of inserting the sequence,
> >         update the call to replace_phi_edge_with_variable.
> >         (match_simplify_replacement): Likewise.
> >         (minmax_replacement): Likewise.
> >         (value_replacement): Create a sequence of statements
> >         which would have defined the ssa name.  Update call
> >         to replace_phi_edge_with_variable.
> >
> > gcc/testsuite/ChangeLog:
> >
> >         PR tree-optimization/101256
> >         * g++.dg/torture/pr101256.C: New test.
> > ---
> >  gcc/dbgcnt.def                          |  1 +
> >  gcc/testsuite/g++.dg/torture/pr101256.C | 28 +++++++++++++
> >  gcc/tree-ssa-phiopt.c                   | 52 ++++++++++++++-----------
> >  3 files changed, 59 insertions(+), 22 deletions(-)
> >  create mode 100644 gcc/testsuite/g++.dg/torture/pr101256.C
> >
> > diff --git a/gcc/dbgcnt.def b/gcc/dbgcnt.def
> > index 93e7b4fd30e..2345899ba68 100644
> > --- a/gcc/dbgcnt.def
> > +++ b/gcc/dbgcnt.def
> > @@ -183,6 +183,7 @@ DEBUG_COUNTER (lim)
> >  DEBUG_COUNTER (local_alloc_for_sched)
> >  DEBUG_COUNTER (match)
> >  DEBUG_COUNTER (merged_ipa_icf)
> > +DEBUG_COUNTER (phiopt_edge_range)
> >  DEBUG_COUNTER (postreload_cse)
> >  DEBUG_COUNTER (pre)
> >  DEBUG_COUNTER (pre_insn)
> > diff --git a/gcc/testsuite/g++.dg/torture/pr101256.C b/gcc/testsuite/g++.dg/torture/pr101256.C
> > new file mode 100644
> > index 00000000000..973a8b4caf3
> > --- /dev/null
> > +++ b/gcc/testsuite/g++.dg/torture/pr101256.C
> > @@ -0,0 +1,28 @@
> > +// { dg-do run }
> > +
> > +template<class T>
> > +const T& max(const T& a, const T& b)
> > +{
> > +    return (a < b) ? b : a;
> > +}
> > +
> > +signed char var_5 = -128;
> > +unsigned int var_11 = 2144479212U;
> > +unsigned long long int arr [22];
> > +
> > +void
> > +__attribute__((noipa))
> > +test(signed char var_5, unsigned var_11) {
> > +  for (short i_61 = 0; i_61 < var_5 + 149; i_61 += 10000)
> > +    arr[i_61] = max((signed char)0, var_5) ? max((signed char)1, var_5) : var_11;
> > +}
> > +
> > +int main() {
> > +  for (int i_0 = 0; i_0 < 22; ++i_0)
> > +      arr [i_0] = 11834725929543695741ULL;
> > +
> > +  test(var_5, var_11);
> > +  if (arr [0] != 2144479212ULL && arr [0] != 11834725929543695741ULL)
> > +    __builtin_abort ();
> > +  return 0;
> > +}
> > diff --git a/gcc/tree-ssa-phiopt.c b/gcc/tree-ssa-phiopt.c
> > index ab12e85569d..71f0019d877 100644
> > --- a/gcc/tree-ssa-phiopt.c
> > +++ b/gcc/tree-ssa-phiopt.c
> > @@ -50,6 +50,7 @@ along with GCC; see the file COPYING3.  If not see
> >  #include "gimple-fold.h"
> >  #include "internal-fn.h"
> >  #include "gimple-range.h"
> > +#include "dbgcnt.h"
> >
> >  static unsigned int tree_ssa_phiopt_worker (bool, bool, bool);
> >  static bool two_value_replacement (basic_block, basic_block, edge, gphi *,
> > @@ -73,7 +74,8 @@ static bool cond_store_replacement (basic_block, basic_block, edge, edge,
> >                                     hash_set<tree> *);
> >  static bool cond_if_else_store_replacement (basic_block, basic_block, basic_block);
> >  static hash_set<tree> * get_non_trapping ();
> > -static void replace_phi_edge_with_variable (basic_block, edge, gphi *, tree);
> > +static void replace_phi_edge_with_variable (basic_block, edge, gphi *, tree,
> > +                                           gimple_seq = NULL);
> >  static void hoist_adjacent_loads (basic_block, basic_block,
> >                                   basic_block, basic_block);
> >  static bool gate_hoist_loads (void);
> > @@ -382,18 +384,20 @@ tree_ssa_phiopt_worker (bool do_store_elim, bool do_hoist_loads, bool early_p)
> >
> >  /* Replace PHI node element whose edge is E in block BB with variable NEW.
> >     Remove the edge from COND_BLOCK which does not lead to BB (COND_BLOCK
> > -   is known to have two edges, one of which must reach BB).  */
> > +   is known to have two edges, one of which must reach BB).
> > +   Optionally insert stmts before the old condition.  */
> >
> >  static void
> >  replace_phi_edge_with_variable (basic_block cond_block,
> > -                               edge e, gphi *phi, tree new_tree)
> > +                               edge e, gphi *phi, tree new_tree,
> > +                               gimple_seq stmts)
> >  {
> >    basic_block bb = gimple_bb (phi);
> >    basic_block block_to_remove;
> >    gimple_stmt_iterator gsi;
> >    tree phi_result = PHI_RESULT (phi);
> >
> > -  /* Duplicate range info if we're the only things setting the target PHI.
> > +  /* Duplicate range info if they are the only things setting the target PHI.
> >       This is needed as later on, the new_tree will be replacing
> >       The assignement of the PHI.
> >       For an example:
> > @@ -401,19 +405,23 @@ replace_phi_edge_with_variable (basic_block cond_block,
> >       _4 = min<a_1, 255>
> >       goto bb2
> >
> > -     range<-INF,255>
> > +     # RANGE [-INF, 255]
> >       a_3 = PHI<_4(1)>
> >       bb3:
> >
> >       use(a_3)
> > -     And _4 gets prograted into the use of a_3 and losing the range info.
> > -     This can't be done for more than 2 incoming edges as the progration
> > -     won't happen.  */
> > +     And _4 gets propagated into the use of a_3 and losing the range info.
> > +     This can't be done for more than 2 incoming edges as the propagation
> > +     won't happen.
> > +     This also cannot be done if the name is not defined by statements to
> > +     be placed before the conditional.  */
> >    if (TREE_CODE (new_tree) == SSA_NAME
> >        && EDGE_COUNT (gimple_bb (phi)->preds) == 2
> >        && INTEGRAL_TYPE_P (TREE_TYPE (phi_result))
> >        && !SSA_NAME_RANGE_INFO (new_tree)
> > -      && SSA_NAME_RANGE_INFO (phi_result))
> > +      && SSA_NAME_RANGE_INFO (phi_result)
> > +      && stmts != NULL
> > +      && dbg_cnt (phiopt_edge_range))
> >      duplicate_ssa_name_range_info (new_tree,
> >                                    SSA_NAME_RANGE_TYPE (phi_result),
> >                                    SSA_NAME_RANGE_INFO (phi_result));
> > @@ -443,6 +451,8 @@ replace_phi_edge_with_variable (basic_block cond_block,
> >
> >    /* Eliminate the COND_EXPR at the end of COND_BLOCK.  */
> >    gsi = gsi_last_bb (cond_block);
> > +  if (stmts)
> > +    gsi_insert_seq_before (&gsi, stmts, GSI_SAME_STMT);
> >    gsi_remove (&gsi, true);
> >
> >    statistics_counter_event (cfun, "Replace PHI with variable", 1);
> > @@ -802,10 +812,8 @@ two_value_replacement (basic_block cond_bb, basic_block middle_bb,
> >    else
> >      new_rhs = gimple_build (&stmts, MINUS_EXPR, type, arg, lhs);
> >    new_rhs = gimple_convert (&stmts, TREE_TYPE (arg0), new_rhs);
> > -  gimple_stmt_iterator gsi = gsi_for_stmt (stmt);
> > -  gsi_insert_seq_before (&gsi, stmts, GSI_SAME_STMT);
> >
> > -  replace_phi_edge_with_variable (cond_bb, e1, phi, new_rhs);
> > +  replace_phi_edge_with_variable (cond_bb, e1, phi, new_rhs, stmts);
> >
> >    /* Note that we optimized this PHI.  */
> >    return true;
> > @@ -920,10 +928,8 @@ 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));
> >      }
> > -  if (seq)
> > -    gsi_insert_seq_before (&gsi, seq, GSI_SAME_STMT);
> >
> > -  replace_phi_edge_with_variable (cond_bb, e1, phi, result);
> > +  replace_phi_edge_with_variable (cond_bb, e1, phi, result, seq);
> >
> >    /* Add Statistic here even though replace_phi_edge_with_variable already
> >       does it as we want to be able to count when match-simplify happens vs
> > @@ -1396,6 +1402,7 @@ value_replacement (basic_block cond_bb, basic_block middle_bb,
> >                       && absorbing_element_p (code_def,
> >                                               cond_rhs, false, rhs2))))))
> >      {
> > +      gimple_seq stmts = NULL;
> >        gsi = gsi_for_stmt (cond);
> >        /* Moving ASSIGN might change VR of lhs, e.g. when moving u_6
> >          def-stmt in:
> > @@ -1417,11 +1424,15 @@ value_replacement (basic_block cond_bb, basic_block middle_bb,
> >           tree plhs = gimple_assign_lhs (prep_stmt[i]);
> >           reset_flow_sensitive_info (plhs);
> >           gsi_from = gsi_for_stmt (prep_stmt[i]);
> > -         gsi_move_before (&gsi_from, &gsi);
> > +         gsi_remove (&gsi_from, false);
> > +
> > +         gimple_seq_add_stmt (&stmts, prep_stmt[i]);
> >         }
> >        gsi_from = gsi_for_stmt (assign);
> > -      gsi_move_before (&gsi_from, &gsi);
> > -      replace_phi_edge_with_variable (cond_bb, e1, phi, lhs);
> > +      gsi_remove (&gsi_from, false);
> > +      gimple_seq_add_stmt (&stmts, assign);
> > +
> > +      replace_phi_edge_with_variable (cond_bb, e1, phi, lhs, stmts);
> >        return 2;
> >      }
> >
> > @@ -1811,10 +1822,7 @@ minmax_replacement (basic_block cond_bb, basic_block middle_bb,
> >    tree phi_result = PHI_RESULT (phi);
> >    result = gimple_build (&stmts, minmax, TREE_TYPE (phi_result), arg0, arg1);
> >
> > -  gsi = gsi_last_bb (cond_bb);
> > -  gsi_insert_seq_before (&gsi, stmts, GSI_NEW_STMT);
> > -
> > -  replace_phi_edge_with_variable (cond_bb, e1, phi, result);
> > +  replace_phi_edge_with_variable (cond_bb, e1, phi, result, stmts);
> >
> >    return true;
> >  }
> > --
> > 2.27.0
> >