[PATCH] D79830: Add support of __builtin_expect_with_probability

Tue Jun 23 12:54:09 PDT 2020

LukeZhuang added a comment.

In D79830#2109324 <https://reviews.llvm.org/D79830#2109324>, @erichkeane wrote:

> @LukeZhuang : This patch causes the buildbots to fail, as O1 <https://reviews.llvm.org/owners/package/1/> means something slightly different with the new pass manager :
>  http://lab.llvm.org:8011/builders/clang-x86_64-debian-new-pass-manager-fast/builds/10542/steps/test-check-all/logs/FAIL%3A%20Clang%3A%3Abuiltin-expect-with-probability.cpp
>
> I'm sorry I didn't notice it during review, but the test that is failing is a poorly written test.  The CFE tests shouldn't be written in a way that depends on the actions of the optimizer, so testing the branch_weights is incorrect.
>
> Please submit a new patch with a way to validate the clang codegen actions without depending on the optimization (that is, would work with -disable-llvm-passes), and if necessary, add a test to llvm to ensure the proper result is validated.
>
> EDIT: To clarify, I've unblocked the buildbots by doing a temporary fix.  But this test needs to be rewritten.

Thank you for pointing out! I have fixed by mimic the test case builtin-expect.c does. It makes sense to test generated llvm intrinsic instead of branch weight here. The lowering to branch weight is already tested in llvm side.

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D79830/new/

https://reviews.llvm.org/D79830