[Bug tree-optimization/110852] [14 Regression] ICE: in get_predictor_value, at predict.cc:2695 with -O -fno-tree-fre and __builtin_expect() since r14-2219-geab57b825bcc35

["jakub at gcc dot gnu.org" <gcc-bugzilla@gcc.gnu.org>]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=110852

Jakub Jelinek <jakub at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |jakub at gcc dot gnu.org

--- Comment #5 from Jakub Jelinek <jakub at gcc dot gnu.org> ---
So, what happens here is that expr_expected_value_1 is called on a binary
operation (GT_EXPR in this case, but that is irrelevant) with 2 SSA_NAME
operands, where one SSA_NAME is set to an INTEGER_CST (not propagated because
of -fno-tree-fre) and the other one result of __builtin_expect.  For the
INTEGER_CST, we get *predictor PRED_UNCONDITIONAL with *probability -1, for
__builtin_expect predictor2 PRED_BUILTIN_EXPECT with probability2 9000.
Next we try to merge the predictors.  As at least one of them has probability
!= -1,
          /* Combine binary predictions.  */
          if (*probability != -1 || probability2 != -1)
            {
              HOST_WIDE_INT p1 = get_predictor_value (*predictor,
*probability);
              HOST_WIDE_INT p2 = get_predictor_value (predictor2,
probability2);
              *probability = RDIV (p1 * p2, REG_BR_PROB_BASE);
            }
is done to combine the value and we get 9000 out of that in this case,
but then pick the predictor with the smaller value which is PRED_UNCONDITIONAL
in:
          if (predictor2 < *predictor)
            *predictor = predictor2;
which causes the later ICE, because only PRED_BUILTIN_EXPECT* should have
probability
other than -1.
Now, given the combination code I wrote:
--- gcc/predict.cc.jj   2024-01-03 11:51:32.000000000 +0100
+++ gcc/predict.cc      2024-01-04 14:04:40.996639979 +0100
@@ -2626,10 +2626,20 @@ expr_expected_value_1 (tree type, tree o
            {
              HOST_WIDE_INT p1 = get_predictor_value (*predictor,
*probability);
              HOST_WIDE_INT p2 = get_predictor_value (predictor2,
probability2);
+             if (*probability != -1 && probability2 != -1)
+               {
+                 /* If both predictors are PRED_BUILTIN_EXPECT*, pick the
+                    smaller one from them.  */
+                 if (predictor2 < *predictor)
+                   *predictor = predictor2;
+               }
+             /* Otherwise, if at least one predictor is PRED_BUILTIN_EXPECT*,
+                use that one for the combination.  */
+             else if (probability2 != -1)
+               *predictor = predictor2;
              *probability = RDIV (p1 * p2, REG_BR_PROB_BASE);
            }
-
-         if (predictor2 < *predictor)
+         else if (predictor2 < *predictor)
            *predictor = predictor2;

          return res;
which fixes the ICE by preferring PRED_BUILTIN_EXPECT* over others.
At least in this case when one operand is a constant and another one is
__builtin_expect* result that seems like the right choice to me, the fact that
one operand is constant doesn't mean the outcome of the binary operation is
unconditionally constant when the other operand is __builtin_expect* based.
But reading the
"This incorrectly takes precedence over more reliable heuristics predicting
that call
to cold noreturn is likely not going to happen."
in the description makes me wonder whether that is what we want always (though,
I must say I don't understand the cold noreturn argument because
PRED_COLD_FUNCTION is never returned from expr_expected_value_1.  But
PRED_COMPARE_AND_SWAP (in between
PRED_BUILTIN_EXPECT_WITH_PROBABILITY and PRED_BUILTIN_EXPECT), whatever the
fortran IFN_BUILTIN_EXPECTs have (all above PRED_BUILTIN_EXPECT*),
PRED_MALLOC_NONNULL (above PRED_BUILTIN_EXPECT*) can appear.
So, shall we prefer PRED_BUILTIN_EXPECT* over PRED_UNCONDITIONAL for the binary
operations, but prefer PRED_COMPARE_AND_SWAP over PRED_BUILTIN_EXPECT (and then
set *probability to -1 obviously)?  The others don't matter because they have
higher value and so aren't picked up.

The reason this doesn't ICE with -fno-tree-fre is that if one of the binary
operands is already INTEGER_CST, it just uses the predictor from the other
operand (which is another argument in support of ignoring PRED_UNCONDITIONAL
operand vs. other predictors when merging).

Oh, another thing is that before the patch there were two spots that merged the
predictors, one for the binary operands (which previously picked the weaker
predictor and now picks stronger predictor), but also in the PHI handling
(which still picks weaker predictor); shouldn't that be changed to match,
including the != -1 handling?