> Am 11.01.2025 um 11:29 schrieb Andrew Pinski <pins...@gmail.com>:
>
> 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.
Hmm, I see. So this is effectively setting likeliness to value range parts. I
can see how this is indeed useful information we shoulda’t dispose of. The
question is whether we can heuristically decide if it’s useful from the PHI def
uses? I suppose your patch is OK as-is.
Richard
>
>>
>>>
>>> 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
>>>
