Add a new builtin-function __builtin_expect_with_probability and intrinsic llvm.expect.with.probability.
The interface is __builtin_expect_with_probability(long expr, long expected, double probability).
It is mainly the same as __builtin_expect besides one more argument indicating the probability of expression equal to expected value. The probability should be a constant floating-point expression and be in range [0.0, 1.0] inclusive.
It is similar to builtin-expect-with-probability function in GCC built-in functions.
Details
Diff Detail
Event Timeline
No OP, but...
First, Overloading builtins is a bit of a pain. You end up having to do custom type checking.
Second, GCC already made the decision to do a separate name. I'd want us to match them unless we have a really good reason not to.
Hi, sorry but I'm not sure what does full context means, is that means I need to show more lines(for example 10 lines) when generating patch or show the whole function? Thank you!
- fix code format
- move probability type and value check from codegen to semacheck
- update patch with full context
clang/include/clang/Basic/Builtins.def | ||
---|---|---|
567 | I assume we don't have any equivalent of the I flag to require a floating-point constant expression? If not, it's probably not worth adding that if this builtin would be the only user of it. | |
clang/lib/CodeGen/CGBuiltin.cpp | ||
2195 | If the intrinsic expects a ConstantFP value, this isn't enough to guarantee that you get one (the set of cases that we fold to constants during expression emission is much smaller than the set of cases we can constant-evaluate). You should run the constant evaluator first, to produce an APFloat, then emit that value as a constant. (Optionally you can form a ConstantExpr as part of the check in Sema and store the value in that object rather than recomputing it here.) | |
clang/lib/Sema/SemaChecking.cpp | ||
1806 | What rules should be applied here? Do we just want to allow anything that we can evaluate (which might change over time), or do we want to use the underlying language's notion of floating-point constant expressions, if it has them? | |
1807 | Converting to a host double here seems suspicious and unnecessary: can you form a suitable APFloat representation of 1.0 and 0.0 instead, and compare to those? | |
1808 | This does not disallow NaN values. I think you want if (!(P >= 0.0 && P <= 1.0)) or equivalent instead. |
clang/lib/CodeGen/CGBuiltin.cpp | ||
---|---|---|
2195 | Thank you for commenting! I have a question about this. If I check this argument can be fold as constant float in Sema, why I need to check it here? Or do you mean I had better create a ConstantFP value here after constant emitting? | |
clang/lib/Sema/SemaChecking.cpp | ||
1806 | Thank you for commenting. I just read the llvm::Expr document and found that it just have isIntegerConstantExpr without a float-point version. Under EvaluateAsFloat it says "Return true if this is a constant which we can fold and convert to a floating point value", thus I use this function. Is there any better function to achieve this goal? |
clang/lib/Sema/SemaChecking.cpp | ||
---|---|---|
1806 | Hi, and I didn't find other builtin functions ever handle case like this, is there any example I could refer to? |
clang/lib/CodeGen/CGBuiltin.cpp | ||
---|---|---|
2195 | EmitScalarExpr will emit arbitrary IR that evaluates the argument. If the argument isn't a simple floating-point literal, then you won't necessarily get an IR-level constant back from that. For example: struct die { constexpr int sides() { return 6; } int roll(); } d6; bool rolled_a_one = __builtin_expect_with_probability(d6.roll(), 1, 1.0 / d6.sides()); Here, we can evaluate 1.0 / d6.sides(), but EmitScalarExpr will emit a function call and a division, not a constant. Instead, you could use EvaluateAsFloat here (you can assert it succeeds because you already checked that in Sema) and then directly form a ConstantFP from the APFloat result. | |
clang/lib/Sema/SemaChecking.cpp | ||
1806 | I think this is the first builtin (and maybe the first part of Clang) that wants to enforce that it sees a floating-point constant expression. The right thing to do is generally to call EvaluateAsConstantExpr and check that it produces the right kind of value and doesn't generate any notes. See the code at the end of CheckConvertedConstantExpr for how to do this. You also need to check that ProbArg is not value-dependent before you try to evaluate it; it's not meaningful to ask for the value of a value-dependent expression. | |
1814–1815 | EvaluateAsConstantExpr will give you some notes to attach to this diagnostic to explain why the expression is non-constant. |
updated 05/15/2020:
(1) add documents about __builtin_expect_with_probability
(2) modify SemaChecking to handle evaluate constant floating-point expression
(3) modify CGBuiltin to evaluate constant floating-point argument before passing it
clang/include/clang/Basic/Builtins.def | ||
---|---|---|
567 | Seems there is currently no tag for constant floating-point | |
clang/lib/CodeGen/CGBuiltin.cpp | ||
2195 | Fixed in lastest patch | |
clang/lib/Sema/SemaChecking.cpp | ||
1806 | fixed in lastest patch | |
1807 | fixed in lastest patch | |
1808 | fixed in lastest patch | |
1814–1815 | fixed in lastest patch |
clang/lib/CodeGen/CGBuiltin.cpp | ||
---|---|---|
2197 | You likely need to assert on the return value (as Richard suggested). | |
2202 | I believe in EvaluateAsFloat you need to pass 'allow side effects', because by default it does not. | |
2204 | Do you still need to evaluate this for side-effects even when called with O1? I think this code only evaluates side-effects in O0 mode at the moment. | |
clang/lib/Sema/SemaChecking.cpp | ||
1805 | Why is value-dependent an error? The expression should be able to be a template parameter. For example: struct S { static constexpr float value = 1.1; }; template<typename T> __builtin_expect_with_probability(b, 1, T::value); } should work. Additionally, in C++20(though not yet implemented in clang it seems): template<float F> __builtin_expect_with_probability(b, 1, F); } Additionally, how about an integer type? It would seem that I should be able ot do: __builtin_expect_with_probability(b, 1, I); // probability is obviously 0 or 1 } | |
1812 | BTW, this check should be valid (ensuring its a float), since normal conversions should happen as a part of the function call. |
clang/lib/CodeGen/CGBuiltin.cpp | ||
---|---|---|
2204 | Hi, sorry I am not sure about it. Since this part of code is after evaluating all three arguments(including evaluate ProbArg with allowing side effect), I think it will evaluate ProbArg with side effect whatever the optimization level is. This part of code is just early return the value of first argument ArgValue and do not generate code to backend. Do I correctly understand this? Thank you! | |
clang/lib/Sema/SemaChecking.cpp | ||
1805 | Hi, this code is based on Richard's suggestion that checking ProbArg is not value-dependent before evaluate. I also saw EvaluateAsFloat source code used in CGBuiltin.cpp that it firstly assert the expression is not value-dependent as following: bool Expr::EvaluateAsFloat(APFloat &Result, const ASTContext &Ctx, SideEffectsKind AllowSideEffects, bool InConstantContext) const { assert(!isValueDependent() && "Expression evaluator can't be called on a dependent expression."); In fact I am not sure is there anything special should be done when is value-dependent. Do you have suggestions about this? Thank you! |
clang/lib/CodeGen/CGBuiltin.cpp | ||
---|---|---|
2202 | Thinking further, Since it is with constant-expressions, we might consider disallowing side effects. | |
2204 | You're right here, I'd read the comment and skipped that ArgValue was evaluated above. | |
clang/lib/Sema/SemaChecking.cpp | ||
1805 | Typically in dependent cases (though this file doesn't deal with the much), we just presume the arguments are valid. The values then get checked when instantiated. Tests to show this would also be necessary. |
clang/lib/Sema/SemaChecking.cpp | ||
---|---|---|
1805 | Hi, is that mean I just not not need this check, and then add test case as you suggest to check it does not yield error, is that correct? Thank you! | |
1812 | Do you mean that when customer pass an integer here and make sure this can handle? I just test this and find isFloat() returns true when this argument is constexpr int. Seems it is automatically converted here. |
FYI: I'm more of a clang contributor, so I'm unable to review the LLVM code, hopefully someone will come along who can check on that.
clang/lib/Sema/SemaChecking.cpp | ||
---|---|---|
1805 | Right, exactly. This code path should end up being evaluated 2x in the case of an instantiation, the first time it is with the dependent placeholder types. The second time it will be instantiated. SO if you cannot validate the rules against dependent values, you just assume it would work and let the 2nd pass catch it. Include some tests to make sure you catch this right and that this is usable in templates. | |
1812 | Ok, thanks for checking. I was unsure, but I presumed that the implicit type-conversion based on the builtin definition would have caused that to happen. |
Updated: 05/21/2020
- add assertion after evaluate probability
- remove value dependent check in SemaChecking
- add test case handling value-dependent template
Additionally, it seems you're missing SEMA tests. Please add those. Those tests should also make sure that the diagnostics still work with dependent values.
clang/include/clang/Basic/Builtins.def | ||
---|---|---|
567 | The spaces after the first parameter are odd/unnecessary. I suspect it is in the line above because it was used to do alignment in the past. Please don't bother putting them in this new line. | |
clang/lib/CodeGen/CGBuiltin.cpp | ||
2197 | This is going to give an unused variable warning in release mode, since assert is a macro. You'll have to cast EvalSucceeded to void after the assert. | |
2199 | First, don't do ==true here. Second, add a string to the assert (see other assert uses). |
1 more nit that I saw (don't use an 'else' after an 'if' branch that returns), but otherwise I think this is good from the CFE perspective. Someone better understanding of LLVM needs to look at the rest though.
clang/lib/Sema/SemaChecking.cpp | ||
---|---|---|
1816 | You don't need the else, since the above branch returns. |
updated: 05/28/2020
(1) remove redundant "else" according to coding standard
(2) change a few document words
Thank you Erich! I think removing this "else" here makes sense.
clang/lib/Sema/SemaChecking.cpp | ||
---|---|---|
1818–1819 | This doesn't look right to me, but maybe i'm misreading something? Consider: if (!(-1 >= llvm::APFloat(0.0) || -1 <= llvm::APFloat(1.0))) { if (!(false || true) { if (false) { so i'd think Probability = -1 would not get diagnosed? | |
clang/test/Sema/builtin-expect-with-probability.cpp | ||
19–20 ↗ | (On Diff #266901) | Do these tests actually pass? |
clang/lib/Sema/SemaChecking.cpp | ||
---|---|---|
1818–1819 | Yes, you're right.. Because of version problem, I still use converting to double in my local build, and miscopy this when upstreaming. Thank you for point it out. | |
clang/test/Sema/builtin-expect-with-probability.cpp | ||
19–20 ↗ | (On Diff #266901) | It pass it for now. Again my local build still use converting to double to compare and passed the test, but I miscopied it when upstream the change. Thank you! |
clang/include/clang/Basic/DiagnosticSemaKinds.td | ||
---|---|---|
10781–10784 | I think "probability argument to" rather than "probability of" would be clearer. | |
clang/lib/CodeGen/CGBuiltin.cpp | ||
2202 | (This is marked as done, but SE_AllowSideEffects is still passed above.) | |
clang/lib/Sema/SemaChecking.cpp | ||
1818–1819 | I think this will probably only work if the target double type is IEEEdouble. (Otherwise you'll be comparing an IEEEdouble APFloat to a different kind of APFloat.) We do not guarantee that double is IEEEdouble for all our supported targets. In particular, for TCE and for AVR (at least under -mdouble=32), double is IEEEsingle instead. It looks like your LLVM intrinsic expects an LLVM IR double (that is, an IEEEdouble) as its third argument, so perhaps the right thing to do would be to convert the APFloat to IEEEdouble here and in CodeGen. Please add a test for a target where double is not IEEEdouble. | |
clang/test/Sema/builtin-expect-with-probability.cpp | ||
18 ↗ | (On Diff #268226) | Please also test some more edge cases. such as: if the probability argument is not convertible to float, or is inf or nan, negative zero, -denorm_min, or 1+epsilon. |
llvm/docs/BranchWeightMetadata.rst | ||
18 | There are no instructions with these names. There are Clang builtin functions with these names and LLVM intrinsics with similar but different names. This document should be documenting the llvm.expect and llvm.expect.with.probability intrinsics. | |
128–135 | This whole section of documentation (line 86 to 166) seems inappropriate to me as LLVM documentation, because it's talking about the behavior of one particular source language with one particular frontend, not LLVM semantics. Here's what I'd suggest:
|
clang/lib/Sema/SemaChecking.cpp | ||
---|---|---|
1818–1819 | Hi, thank you for comments! I have several questions here: | |
llvm/docs/BranchWeightMetadata.rst | ||
128–135 | Yeah, thank you! I think your suggestion makes much sense, it should be in clang. However, the original __builtin_expect part is not written by me and and I am not sure its affect. So I added the author of this part as reviewer and I think it had better to ask his idea about this. |
updated: 06/06/2020
(1) convert argument to IEEEdouble for different target
(2) update tests, add edge cases and llvm test
(3) minor change
added ll test case
clang/lib/Sema/SemaChecking.cpp | ||
---|---|---|
1818–1819 | Sorry, after I test with different targets I understood your suggestion. It shows weird behavior in comparing for AVR target. Now I have converted to IEEEdouble. I also added test for AVR target and passed it. | |
clang/test/Sema/builtin-expect-with-probability.cpp | ||
18 ↗ | (On Diff #268226) | added more edge cases here and example for AVR target |
llvm/docs/BranchWeightMetadata.rst | ||
18 | I also think I might had better add intrinsic documents in llvm/docs/LangRef.rst instead of here, and keep __builtin_exepct_with_probability here or discuss with the original author about this. |
updated: 06/06/2020
(1) updated llvm.expect.with.probability intrinsic document in LangRef.rst
updated: 06/08/2020
(1) update llvm side test for intrinsic llvm.expect.with.probability, which mimics test for llvm.expect
updated: 06/09/2020
(1) improve code in LowerExpectIntrinsic
(2) update and simplify test
Please fix the 'nit' when committing.
clang/lib/CodeGen/CGBuiltin.cpp | ||
---|---|---|
2202 | Nit, move this cast next to the assert, its a common pattern in LLVM, so moving it away makes its purpose pretty unknown. |
clang/lib/CodeGen/CGBuiltin.cpp | ||
---|---|---|
2209 | Fix comment and remove Probability? |
@rsmith Hi, could you please take a look at my revision since last comment? Thank you!
llvm/lib/Transforms/Scalar/LowerExpectIntrinsic.cpp | ||
---|---|---|
62 | FYI: this breaks GCC5: https://buildkite.com/mlir/mlir-core/builds/5841#4a7c245b-9841-44aa-a82a-97686b04b672 |
Hi, I am getting a compiler error due to this patch. I am using cmake command: cmake ../llvm -DCMAKE_BUILD_TYPE=Release -DLLVM_ENABLE_PROJECTS="clang;lld;clang-tools-extra" -DLLVM_TARGETS_TO_BUILD="AMDGPU;X86" -DLLVM_ENABLE_ASSERTIONS=1 . Could you please take a look?
Error:
/root/llvm-project/llvm/lib/Transforms/Scalar/LowerExpectIntrinsic.cpp: In function 'std::tuple<unsigned int, unsigned int> getBranchWeight(llvm::Intrinsic::ID, llvm::CallInst*, int)':
/root/llvm-project/llvm/lib/Transforms/Scalar/LowerExpectIntrinsic.cpp:62:53: error: converting to 'std::tuple<unsigned int, unsigned int>' from initializer list would use explicit constructor 'constexpr std::tuple<_T1, _T2>::tuple(_U1&&, _U2&&) [with _U1 = llvm:🆑:opt<unsigned int>&; _U2 = llvm:🆑:opt<unsigned int>&; <template-parameter-2-3> = void; _T1 = unsigned int; _T2 = unsigned int]'
return {LikelyBranchWeight, UnlikelyBranchWeight}; ^
/root/llvm-project/llvm/lib/Transforms/Scalar/LowerExpectIntrinsic.cpp:74:33: error: converting to 'std::tuple<unsigned int, unsigned int>' from initializer list would use explicit constructor 'constexpr std::tuple<_T1, _T2>::tuple(_U1&&, _U2&&) [with _U1 = unsigned int&; _U2 = unsigned int&; <template-parameter-2-3> = void; _T1 = unsigned int; _T2 = unsigned int]'
return {LikelyBW, UnlikelyBW}; ^
Turns out when I was validating my patch, someone had beaten me to it, it should be fixed, it just wasn't me :)
@LukeZhuang : This patch causes the buildbots to fail, as O1 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.
I assume we don't have any equivalent of the I flag to require a floating-point constant expression? If not, it's probably not worth adding that if this builtin would be the only user of it.