On Sat, Jan 11, 2025 at 2:10 AM Richard Biener
<richard.guent...@gmail.com> wrote:
>
>
>
> > Am 11.01.2025 um 09:49 schrieb Andrew Pinski <quic_apin...@quicinc.com>:
> >
> > In this case, early phiopt would get rid of the user provided predicator
> > for hot/cold as it would remove the basic blocks. The easiest and best 
> > option is
> > for early phi-opt don't do phi-opt if the middle basic-block(s) have either
> > a hot or cold predict statement. Then after inlining, jump threading will 
> > most likely
> > happen and that will keep around the predictor.
> >
> > Note this only needs to be done for match_simplify_replacement and not the 
> > other
> > phi-opt functions because currently only match_simplify_replacement is able 
> > to skip
> > middle bb with predicator statements in it.
> >
> > OK? Bootstrapped and tested on x86_64-linux-gnu.
>
> Hmm, I think we still want to allow abs/min/max replacements early I think.
>
> What kind of replacement is performed in the problematic case?

Replace `a ? true : false` with simple `a`. So should we just reject
that if early and the middle contains a hot/cold predictor?
That is the only case where jump threading will most likely happen
after inlining too.
I will work on that tomorrow.

>
> >
> >    PR tree-optimization/117935
> >
> > gcc/ChangeLog:
> >
> >    * tree-ssa-phiopt.cc (contains_hot_cold_predict): New function.
> >    (match_simplify_replacement): Return early if early_p and one of
> >    the middle bb(s) have a hot/cold predict statement.
> >
> > gcc/testsuite/ChangeLog:
> >
> >    * gcc.dg/predict-24.c: New test.
> >
> > Signed-off-by: Andrew Pinski <quic_apin...@quicinc.com>
> > ---
> > gcc/testsuite/gcc.dg/predict-24.c | 24 +++++++++++++++++++++
> > gcc/tree-ssa-phiopt.cc            | 35 +++++++++++++++++++++++++++++++
> > 2 files changed, 59 insertions(+)
> > create mode 100644 gcc/testsuite/gcc.dg/predict-24.c
> >
> > diff --git a/gcc/testsuite/gcc.dg/predict-24.c 
> > b/gcc/testsuite/gcc.dg/predict-24.c
> > new file mode 100644
> > index 00000000000..b2c8a77c323
> > --- /dev/null
> > +++ b/gcc/testsuite/gcc.dg/predict-24.c
> > @@ -0,0 +1,24 @@
> > +/* { dg-do compile } */
> > +/* { dg-options "-O2 -fdump-tree-profile_estimate" } */
> > +/* PR tree-optimization/117935 */
> > +
> > +static inline bool has_value(bool b)
> > +{
> > +  if (b)
> > +  {
> > +    [[gnu::hot, gnu::unused]] label1:
> > +    return true;
> > +  }
> > +  else
> > +    return false;
> > +}
> > +/* The hot label should last until it gets inlined into value_or and 
> > jump_threaded.  */
> > +int value_or(bool b, int def0, int def1)
> > +{
> > +  if (has_value(b))
> > +    return def0;
> > +  else
> > +    return def1;
> > +}
> > +/* { dg-final { scan-tree-dump-times "first match heuristics: 90.00%" 2 
> > "profile_estimate"} } */
> > +/* { dg-final { scan-tree-dump-times "hot label heuristics of edge 
> > \[0-9\]+->\[0-9]+: 90.00%" 2 "profile_estimate"} } */
> > diff --git a/gcc/tree-ssa-phiopt.cc b/gcc/tree-ssa-phiopt.cc
> > index 64d3ba9e160..8fed3eadbb0 100644
> > --- a/gcc/tree-ssa-phiopt.cc
> > +++ b/gcc/tree-ssa-phiopt.cc
> > @@ -56,6 +56,7 @@ along with GCC; see the file COPYING3.  If not see
> > #include "tree-ssa-dce.h"
> > #include "calls.h"
> > #include "tree-ssa-loop-niter.h"
> > +#include "gimple-predict.h"
> >
> > /* Return the singleton PHI in the SEQ of PHIs for edges E0 and E1. */
> >
> > @@ -916,6 +917,27 @@ auto_flow_sensitive::~auto_flow_sensitive ()
> >     p.second.restore (p.first);
> > }
> >
> > +/* Returns true if BB contains an user provided predictor
> > +   (PRED_HOT_LABEL/PRED_COLD_LABEL).  */
> > +
> > +static bool
> > +contains_hot_cold_predict (basic_block bb)
> > +{
> > +  gimple_stmt_iterator gsi;
> > +  gsi = gsi_start_nondebug_after_labels_bb (bb);
> > +  for (; !gsi_end_p (gsi); gsi_next_nondebug (&gsi))
> > +    {
> > +      gimple *s = gsi_stmt (gsi);
> > +      if (gimple_code (s) != GIMPLE_PREDICT)
> > +    continue;
> > +      auto predict = gimple_predict_predictor (s);
> > +      if (predict == PRED_HOT_LABEL
> > +      || predict == PRED_COLD_LABEL)
> > +    return true;
> > +    }
> > +  return false;
> > +}
> > +
> > /*  The function match_simplify_replacement does the main work of doing the
> >     replacement using match and simplify.  Return true if the replacement 
> > is done.
> >     Otherwise return false.
> > @@ -953,6 +975,19 @@ match_simplify_replacement (basic_block cond_bb, 
> > basic_block middle_bb,
> >                      stmt_to_move_alt))
> >     return false;
> >
> > +  /* For early phiopt, we don't want to lose user generated predictors,
> > +     that is if either middle bb contains either a hot or cold label 
> > predict,
> > +     the phiopt should be skipped. */
> > +  if (early_p)
> > +    {
> > +      if (contains_hot_cold_predict (middle_bb))
> > +    return false;
> > +      if (threeway_p
> > +      && middle_bb != middle_bb_alt
> > +      && contains_hot_cold_predict (middle_bb_alt))
> > +    return false;
> > +    }
> > +
> >   /* Do not make conditional undefs unconditional.  */
> >   if ((TREE_CODE (arg0) == SSA_NAME
> >        && ssa_name_maybe_undef_p (arg0))
> > --
> > 2.43.0
> >

Reply via email to