[PATCH] D79830: Add support of __builtin_expect_with_probability

LukeZhuang added a comment. In D79830#2109324 , @erichkeane wrote: > @LukeZhuang : This patch causes the buildbots to fail, as O1 > means something slightly > different with the new pass manager : > >

[PATCH] D79830: Add support of __builtin_expect_with_probability

erichkeane added a comment. @LukeZhuang : This patch causes the buildbots to fail, as O1 means something slightly different with the new pass manager :

[PATCH] D79830: Add support of __builtin_expect_with_probability

erichkeane added a comment. In D79830#2107404 , @LukeZhuang wrote: > Thank you very much! Turns out when I was validating my patch, someone had beaten me to it, it should be fixed, it just wasn't me :) Repository: rG LLVM Github Monorepo CHANGES

[PATCH] D79830: Add support of __builtin_expect_with_probability

LukeZhuang added a comment. Thank you very much! Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D79830/new/ https://reviews.llvm.org/D79830 ___ cfe-commits mailing list cfe-commits@lists.llvm.org

[PATCH] D79830: Add support of __builtin_expect_with_probability

erichkeane added a comment. I'll get this fixed, thanks both of you for letting me know. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D79830/new/ https://reviews.llvm.org/D79830 ___ cfe-commits

[PATCH] D79830: Add support of __builtin_expect_with_probability

ashi1 added subscribers: yaxunl, ashi1. ashi1 added a comment. 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"

[PATCH] D79830: Add support of __builtin_expect_with_probability

antiagainst added inline comments. Comment at: llvm/lib/Transforms/Scalar/LowerExpectIntrinsic.cpp:62 +// __builtin_expect +return {LikelyBranchWeight, UnlikelyBranchWeight}; + } else { FYI: this breaks GCC5:

[PATCH] D79830: Add support of __builtin_expect_with_probability

This revision was automatically updated to reflect the committed changes. Closed by commit rG37fb86030127: Add support of __builtin_expect_with_probability (authored by LukeZhuang, committed by erichkeane). Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION

[PATCH] D79830: Add support of __builtin_expect_with_probability

LukeZhuang added a comment. Herald added a reviewer: jdoerfert. Hi, I do not have access to commit, could anybody help me to commit it? Thank you! CHANGES SINCE LAST ACTION https://reviews.llvm.org/D79830/new/ https://reviews.llvm.org/D79830

[PATCH] D79830: Add support of __builtin_expect_with_probability

LukeZhuang added a comment. @rsmith Hi, could you please take a look at my revision since last comment? Thank you! CHANGES SINCE LAST ACTION https://reviews.llvm.org/D79830/new/ https://reviews.llvm.org/D79830 ___ cfe-commits mailing list

[PATCH] D79830: Add support of __builtin_expect_with_probability

LukeZhuang updated this revision to Diff 270966. LukeZhuang marked an inline comment as done. LukeZhuang added a comment. Added: fix comment CHANGES SINCE LAST ACTION https://reviews.llvm.org/D79830/new/ https://reviews.llvm.org/D79830 Files: clang/include/clang/Basic/Builtins.def

[PATCH] D79830: Add support of __builtin_expect_with_probability

LukeZhuang updated this revision to Diff 270963. LukeZhuang added a comment. **updated: 06/15/2020** (1) fix minor variable name and doc word CHANGES SINCE LAST ACTION https://reviews.llvm.org/D79830/new/ https://reviews.llvm.org/D79830 Files: clang/include/clang/Basic/Builtins.def

[PATCH] D79830: Add support of __builtin_expect_with_probability

xbolva00 added inline comments. Comment at: clang/lib/CodeGen/CGBuiltin.cpp:2216 +// it could have side-effects. +if (CGM.getCodeGenOpts().OptimizationLevel == 0) + return RValue::get(ArgValue); Fix comment and remove Probability? CHANGES SINCE

[PATCH] D79830: Add support of __builtin_expect_with_probability

LukeZhuang updated this revision to Diff 270937. LukeZhuang marked an inline comment as done. LukeZhuang added a comment. **updated: 06/15/2020** (1) minor change of assertion and cast CHANGES SINCE LAST ACTION https://reviews.llvm.org/D79830/new/ https://reviews.llvm.org/D79830 Files:

[PATCH] D79830: Add support of __builtin_expect_with_probability

LukeZhuang added a comment. In D79830#2093338 , @erichkeane wrote: > Please fix the 'nit' when committing. Fixed. Thank you! CHANGES SINCE LAST ACTION https://reviews.llvm.org/D79830/new/ https://reviews.llvm.org/D79830

[PATCH] D79830: Add support of __builtin_expect_with_probability

erichkeane accepted this revision. erichkeane added a comment. Please fix the 'nit' when committing. Comment at: clang/lib/CodeGen/CGBuiltin.cpp:2207 +llvm::RoundingMode::Dynamic, ); +(void)EvalSucceed; +llvm::Type *Ty =

[PATCH] D79830: Add support of __builtin_expect_with_probability

LukeZhuang added a comment. ping :) For clang side is there any suggestions? Thank you! CHANGES SINCE LAST ACTION https://reviews.llvm.org/D79830/new/ https://reviews.llvm.org/D79830 ___ cfe-commits mailing list cfe-commits@lists.llvm.org

[PATCH] D79830: Add support of __builtin_expect_with_probability

davidxl accepted this revision. davidxl added a comment. This revision is now accepted and ready to land. LLVM side of change LGTM. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D79830/new/ https://reviews.llvm.org/D79830 ___ cfe-commits

[PATCH] D79830: Add support of __builtin_expect_with_probability

LukeZhuang updated this revision to Diff 269642. LukeZhuang added a comment. **updated: 06/09/2020** (1) improve code in LowerExpectIntrinsic (2) update and simplify test CHANGES SINCE LAST ACTION https://reviews.llvm.org/D79830/new/ https://reviews.llvm.org/D79830 Files:

[PATCH] D79830: Add support of __builtin_expect_with_probability

davidxl added inline comments. Comment at: llvm/lib/Transforms/Scalar/LowerExpectIntrinsic.cpp:58 +std::pair setBranchWeight(Intrinsic::ID IntrinsicID, + CallInst *CI, int BranchCount) { nit: change name to

[PATCH] D79830: Add support of __builtin_expect_with_probability

LukeZhuang updated this revision to Diff 269408. LukeZhuang added a comment. **updated: 06/08/2020** (1) update llvm side test for intrinsic llvm.expect.with.probability, which mimics test for llvm.expect CHANGES SINCE LAST ACTION https://reviews.llvm.org/D79830/new/

[PATCH] D79830: Add support of __builtin_expect_with_probability

LukeZhuang updated this revision to Diff 269028. LukeZhuang added a reviewer: dexonsmith. LukeZhuang added a comment. **updated: 06/06/2020** (1) updated llvm.expect.with.probability intrinsic document in LangRef.rst CHANGES SINCE LAST ACTION https://reviews.llvm.org/D79830/new/

[PATCH] D79830: Add support of __builtin_expect_with_probability

LukeZhuang updated this revision to Diff 269002. LukeZhuang added a comment. Herald added subscribers: Jim, dylanmckay. **updated: 06/06/2020** (1) convert argument to IEEEdouble for different target (2) update tests, add edge cases and llvm test (3) minor change CHANGES SINCE LAST ACTION

[PATCH] D79830: Add support of __builtin_expect_with_probability

LukeZhuang marked 12 inline comments as done. LukeZhuang added a comment. In D79830#2075038 , @davidxl wrote: > Can you add some tests at the LLVM side? added ll test case Comment at: clang/lib/Sema/SemaChecking.cpp:1818-1819 +

[PATCH] D79830: Add support of __builtin_expect_with_probability

LukeZhuang added inline comments. Comment at: clang/lib/Sema/SemaChecking.cpp:1818-1819 +llvm::APFloat Probability = Eval.Val.getFloat(); +if (!(Probability >= llvm::APFloat(0.0) && + Probability <= llvm::APFloat(1.0))) { + Diag(ProbArg->getLocStart(),

[PATCH] D79830: Add support of __builtin_expect_with_probability

davidxl added a comment. Can you add some tests at the LLVM side? CHANGES SINCE LAST ACTION https://reviews.llvm.org/D79830/new/ https://reviews.llvm.org/D79830 ___ cfe-commits mailing list cfe-commits@lists.llvm.org

[PATCH] D79830: Add support of __builtin_expect_with_probability

rsmith added inline comments. Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:10804-10807 + "probability of __builtin_expect_with_probability must be constant " + "floating-point expression">; +def err_probability_out_of_range : Error< + "probability of

[PATCH] D79830: Add support of __builtin_expect_with_probability

LukeZhuang marked 4 inline comments as done. LukeZhuang added inline comments. Comment at: clang/lib/Sema/SemaChecking.cpp:1818-1819 +llvm::APFloat Probability = Eval.Val.getFloat(); +if (!(Probability >= llvm::APFloat(0.0) || + Probability <=

[PATCH] D79830: Add support of __builtin_expect_with_probability

LukeZhuang updated this revision to Diff 268226. LukeZhuang added a comment. **updated 06/03/2020**: (1) fix bug of range checking in SemaChecking CHANGES SINCE LAST ACTION https://reviews.llvm.org/D79830/new/ https://reviews.llvm.org/D79830 Files: clang/include/clang/Basic/Builtins.def

[PATCH] D79830: Add support of __builtin_expect_with_probability

lebedev.ri added inline comments. Comment at: clang/lib/Sema/SemaChecking.cpp:1818-1819 +llvm::APFloat Probability = Eval.Val.getFloat(); +if (!(Probability >= llvm::APFloat(0.0) || + Probability <= llvm::APFloat(1.0))) { + Diag(ProbArg->getLocStart(),

[PATCH] D79830: Add support of __builtin_expect_with_probability

LukeZhuang added a comment. ping :) CHANGES SINCE LAST ACTION https://reviews.llvm.org/D79830/new/ https://reviews.llvm.org/D79830 ___ cfe-commits mailing list cfe-commits@lists.llvm.org

[PATCH] D79830: Add support of __builtin_expect_with_probability

LukeZhuang updated this revision to Diff 266901. LukeZhuang added a comment. **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. CHANGES SINCE LAST ACTION

[PATCH] D79830: Add support of __builtin_expect_with_probability

erichkeane added a comment. 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. Comment at:

[PATCH] D79830: Add support of __builtin_expect_with_probability

LukeZhuang updated this revision to Diff 266285. LukeZhuang added a comment. **updated: 05/26/2020** (1) add Sema test (2) improve assert (3) format change CHANGES SINCE LAST ACTION https://reviews.llvm.org/D79830/new/ https://reviews.llvm.org/D79830 Files:

[PATCH] D79830: Add support of __builtin_expect_with_probability

LukeZhuang marked 3 inline comments as done. LukeZhuang added a comment. Fixed in latest update as well as adding test. Thank you! CHANGES SINCE LAST ACTION https://reviews.llvm.org/D79830/new/ https://reviews.llvm.org/D79830 ___ cfe-commits

[PATCH] D79830: Add support of __builtin_expect_with_probability

erichkeane added a comment. 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. Comment at: clang/include/clang/Basic/Builtins.def:567 BUILTIN(__builtin_expect,

[PATCH] D79830: Add support of __builtin_expect_with_probability

LukeZhuang marked 8 inline comments as done. LukeZhuang added a comment. In D79830#2049582 , @erichkeane wrote: > 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.

[PATCH] D79830: Add support of __builtin_expect_with_probability

LukeZhuang updated this revision to Diff 265598. LukeZhuang added a comment. **Updated: 05/21/2020** 1. add assertion after evaluate probability 2. remove value dependent check in SemaChecking 3. add test case handling value-dependent template CHANGES SINCE LAST ACTION

[PATCH] D79830: Add support of __builtin_expect_with_probability

erichkeane added a comment. 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. Comment at: clang/lib/Sema/SemaChecking.cpp:1805 +const Expr *ProbArg = TheCall->getArg(2); +if

[PATCH] D79830: Add support of __builtin_expect_with_probability

LukeZhuang marked 6 inline comments as done. LukeZhuang added inline comments. Comment at: clang/lib/Sema/SemaChecking.cpp:1805 +const Expr *ProbArg = TheCall->getArg(2); +if (ProbArg->isValueDependent()) + return ExprError(); erichkeane wrote: >

[PATCH] D79830: Add support of __builtin_expect_with_probability

erichkeane added inline comments. Comment at: clang/lib/CodeGen/CGBuiltin.cpp:2202 +// won't use it for anything. +// Note, we still IRGen ExpectedValue because it could have side-effects. +if (CGM.getCodeGenOpts().OptimizationLevel == 0) erichkeane

[PATCH] D79830: Add support of __builtin_expect_with_probability

LukeZhuang added inline comments. Comment at: clang/lib/CodeGen/CGBuiltin.cpp:2204 +if (CGM.getCodeGenOpts().OptimizationLevel == 0) + return RValue::get(ArgValue); + erichkeane wrote: > Do you still need to evaluate this for side-effects even when

[PATCH] D79830: Add support of __builtin_expect_with_probability

erichkeane added inline comments. Comment at: clang/lib/CodeGen/CGBuiltin.cpp:2197 +const Expr *ProbArg = E->getArg(2); +ProbArg->EvaluateAsFloat(Probability, CGM.getContext()); +llvm::Type *Ty = ConvertType(ProbArg->getType()); You likely need to

[PATCH] D79830: Add support of __builtin_expect_with_probability

LukeZhuang updated this revision to Diff 264357. LukeZhuang added a comment. 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

[PATCH] D79830: Add support of __builtin_expect_with_probability

LukeZhuang marked 15 inline comments as done. LukeZhuang added inline comments. Comment at: clang/include/clang/Basic/Builtins.def:567 BUILTIN(__builtin_expect, "LiLiLi" , "nc") +BUILTIN(__builtin_expect_with_probability, "LiLiLid" , "nc") BUILTIN(__builtin_prefetch,

[PATCH] D79830: Add support of __builtin_expect_with_probability

rsmith added inline comments. Comment at: clang/lib/CodeGen/CGBuiltin.cpp:2195 +Value *ExpectedValue = EmitScalarExpr(E->getArg(1)); +Value *Confidence = EmitScalarExpr(E->getArg(2)); +// Don't generate llvm.expect.with.probability on -O0 as the backend

[PATCH] D79830: Add support of __builtin_expect_with_probability

LukeZhuang added inline comments. Comment at: clang/lib/Sema/SemaChecking.cpp:1805 +const Expr *ProbArg = TheCall->getArg(2); +if (ProbArg->EvaluateAsFloat(Confidence, Context)) { + double P = Confidence.convertToDouble(); LukeZhuang wrote: > rsmith

[PATCH] D79830: Add support of __builtin_expect_with_probability

LukeZhuang added inline comments. Comment at: clang/lib/CodeGen/CGBuiltin.cpp:2195 +Value *ExpectedValue = EmitScalarExpr(E->getArg(1)); +Value *Confidence = EmitScalarExpr(E->getArg(2)); +// Don't generate llvm.expect.with.probability on -O0 as the backend

[PATCH] D79830: Add support of __builtin_expect_with_probability

rsmith added inline comments. Comment at: clang/include/clang/Basic/Builtins.def:567 BUILTIN(__builtin_expect, "LiLiLi" , "nc") +BUILTIN(__builtin_expect_with_probability, "LiLiLid" , "nc") BUILTIN(__builtin_prefetch, "vvC*.", "nc") I assume we don't have

[PATCH] D79830: Add support of __builtin_expect_with_probability

LukeZhuang added a comment. In D79830#2034796 , @lebedev.ri wrote: > In D79830#2034779 , @LukeZhuang > wrote: > > > In D79830#2033367 , @lebedev.ri > > wrote: > > > > >

[PATCH] D79830: Add support of __builtin_expect_with_probability

LukeZhuang marked 4 inline comments as done. LukeZhuang added inline comments. Comment at: clang/lib/CodeGen/CGBuiltin.cpp:2049 +ConstantFP *Confidence_f = dyn_cast(Confidence); +if (!Confidence_f) { + CGM.Error(E->getArg(2)->getLocStart(),

[PATCH] D79830: Add support of __builtin_expect_with_probability

LukeZhuang updated this revision to Diff 263877. LukeZhuang added a comment. 1. fix code format 2. move probability type and value check from codegen to semacheck 3. update patch with full context CHANGES SINCE LAST ACTION https://reviews.llvm.org/D79830/new/ https://reviews.llvm.org/D79830

[PATCH] D79830: Add support of __builtin_expect_with_probability

erichkeane added a comment. In D79830#2034779 , @LukeZhuang wrote: > In D79830#2033367 , @lebedev.ri > wrote: > > > Thanks for working on this. > > Please upload patch with full context. > > > Hi, sorry but I'm

[PATCH] D79830: Add support of __builtin_expect_with_probability

lebedev.ri added a comment. In D79830#2034779 , @LukeZhuang wrote: > In D79830#2033367 , @lebedev.ri > wrote: > > > Thanks for working on this. > > Please upload patch with full context. > > > Hi, sorry but I'm

[PATCH] D79830: Add support of __builtin_expect_with_probability

LukeZhuang added a comment. In D79830#2033367 , @lebedev.ri wrote: > Thanks for working on this. > Please upload patch with full context. Hi, sorry but I'm not sure what does full context means, is that means I need to show more lines(for example 10

[PATCH] D79830: Add support of __builtin_expect_with_probability

xbolva00 added a comment. In D79830#2034380 , @RKSimon wrote: > Add a description in our documentation? And Ideally add a note to release notes too.. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION

[PATCH] D79830: Add support of __builtin_expect_with_probability

erichkeane added a comment. In D79830#2034400 , @davidxl wrote: > Is it possible to overload __builtin_expect(..)? 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

[PATCH] D79830: Add support of __builtin_expect_with_probability

davidxl added a comment. Is it possible to overload __builtin_expect(..)? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D79830/new/ https://reviews.llvm.org/D79830 ___ cfe-commits mailing list

[PATCH] D79830: Add support of __builtin_expect_with_probability

RKSimon added a comment. Add a description in our documentation? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D79830/new/ https://reviews.llvm.org/D79830 ___ cfe-commits mailing list

[PATCH] D79830: Add support of __builtin_expect_with_probability

erichkeane added a comment. Type checking/value checking for this should happen during Sema, not codegen. Comment at: clang/lib/CodeGen/CGBuiltin.cpp:2049 +ConstantFP *Confidence_f = dyn_cast(Confidence); +if (!Confidence_f) { +

[PATCH] D79830: Add support of __builtin_expect_with_probability

lebedev.ri added a reviewer: erichkeane. lebedev.ri added a comment. Thanks for working on this. Please upload patch with full context. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D79830/new/ https://reviews.llvm.org/D79830

[PATCH] D79830: Add support of __builtin_expect_with_probability

LukeZhuang created this revision. LukeZhuang added reviewers: RKSimon, phosek, gribozavr, davidxl, chandlerc, pete, jyknight, dblaikie, kuba. LukeZhuang added projects: LLVM, clang. Herald added subscribers: llvm-commits, cfe-commits, jdoerfert, hiraditya. Add a new builtin-function