]> git.ipfire.org Git - thirdparty/gcc.git/commit
Fix handling of __builtin_expect_with_probability and improve first-match heuristics
authorJan Hubicka <jh@suse.cz>
Fri, 30 Jun 2023 14:27:27 +0000 (16:27 +0200)
committerJan Hubicka <jh@suse.cz>
Fri, 30 Jun 2023 14:27:27 +0000 (16:27 +0200)
commiteab57b825bcc350e9ff44eb2fa739a80199d9bb1
treeb7327ce2419a7818d9a11062e814400d2a30abf4
parentef4ea6e08707d27674a8c5ddb4b478aac8713c03
Fix handling of __builtin_expect_with_probability and improve first-match heuristics

While looking into the std::vector _M_realloc_insert codegen I noticed that
call of __throw_bad_alloc is predicted with 10% probability. This is because
the conditional guarding it has __builtin_expect (cond, 0) on it.  This
incorrectly takes precedence over more reliable heuristics predicting that call
to cold noreturn is likely not going to happen.

So I reordered the predictors so __builtin_expect_with_probability comes first
after predictors that never makes a mistake (so user can use it to always
specify the outcome by hand).  I also downgraded malloc predictor since I do
think user-defined malloc functions & new operators may behave funny ways and
moved usual __builtin_expect after the noreturn cold predictor.

This triggered latent bug in expr_expected_value_1 where

  if (*predictor < predictor2)
      *predictor = predictor2;

should be:

    if (predictor2 < *predictor)
      *predictor = predictor2;

which eventually triggered an ICE on combining heuristics.  This made me notice
that we can do slightly better while combining expected values in case only
one of the parameters (such as in a*b when we expect a==0) can determine
overall result.

Note that the new code may pick weaker heuristics in case that both values are
predicted.  Not sure if this scenario is worth the extra CPU time: there is
not correct way to combine the probabilities anyway since we do not know if
the predictions are independent, so I think users should not rely on it.

Fixing this issue uncovered another problem.  In 2018 Martin Liska added
code predicting that MALLOC returns non-NULL but instead of that he predicts
that it returns true (boolean 1).  This sort of works for testcase testing
 malloc (10) != NULL
but, for example, we will predict
 malloc (10) == malloc (10)
as true, which is not right and such comparsion may happen in real code

I think proper way is to update expr_expected_value_1 to work with value
ranges, but that needs greater surgery so I decided to postpone this and
only add FIXME and fill PR110499.

gcc/ChangeLog:

PR middle-end/109849
* predict.cc (estimate_bb_frequencies): Turn to static function.
(expr_expected_value_1): Fix handling of binary expressions with
predicted values.
* predict.def (PRED_MALLOC_NONNULL): Move later in the priority queue.
(PRED_BUILTIN_EXPECT_WITH_PROBABILITY): Move to almost top of the priority
queue.
* predict.h (estimate_bb_frequencies): No longer declare it.

gcc/testsuite/ChangeLog:

PR middle-end/109849
* gcc.dg/predict-18.c: Improve testcase.
gcc/predict.cc
gcc/predict.def
gcc/predict.h
gcc/testsuite/gcc.dg/predict-18.c