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?