[PATCH] D79830: Add support of __builtin_expect_with_probability

[Zhi Zhuang via Phabricator via cfe-commits]

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 called with O1? 
>  I think this code only evaluates side-effects in O0 mode at the moment.
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!



Comment at: clang/lib/Sema/SemaChecking.cpp:1805
+const Expr *ProbArg = TheCall->getArg(2);
+if (ProbArg->isValueDependent())
+  return ExprError();

erichkeane wrote:
> 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
> void f(bool b) {
> __builtin_expect_with_probability(b, 1, T::value);
> }
> 
> should work.
> 
> Additionally, in C++20(though not yet implemented in clang it seems):
> 
> template
> void f(bool b) {
> __builtin_expect_with_probability(b, 1, F);
> }
> 
> Additionally, how about an integer type?  It would seem that I should be able 
> ot do:
> template
> void f(bool b) {
> __builtin_expect_with_probability(b, 1, I); // probability is obviously 0 
> or 1
> }
> 
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!


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

https://reviews.llvm.org/D79830



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D79830: Add support of __builtin_expect_with_probability

[Zhi Zhuang via Phabricator via cfe-commits]

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:
> LukeZhuang wrote:
> > erichkeane wrote:
> > > 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
> > > void f(bool b) {
> > > __builtin_expect_with_probability(b, 1, T::value);
> > > }
> > > 
> > > should work.
> > > 
> > > Additionally, in C++20(though not yet implemented in clang it seems):
> > > 
> > > template
> > > void f(bool b) {
> > > __builtin_expect_with_probability(b, 1, F);
> > > }
> > > 
> > > Additionally, how about an integer type?  It would seem that I should be 
> > > able ot do:
> > > template
> > > void f(bool b) {
> > > __builtin_expect_with_probability(b, 1, I); // probability is 
> > > obviously 0 or 1
> > > }
> > > 
> > 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!
> 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.
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!



Comment at: clang/lib/Sema/SemaChecking.cpp:1812
+  Context)) ||
+!Eval.Val.isFloat()) {
+  Diag(ProbArg->getLocStart(), diag::err_probability_not_constant_float)

erichkeane wrote:
> BTW, this check should be valid (ensuring its a float), since normal 
> conversions should happen as a part of the function call.
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.


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

https://reviews.llvm.org/D79830



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D79830: Add support of __builtin_expect_with_probability

[Zhi Zhuang via Phabricator via cfe-commits]

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
  https://reviews.llvm.org/D79830/new/

https://reviews.llvm.org/D79830

Files:
  clang/include/clang/Basic/Builtins.def
  clang/include/clang/Basic/DiagnosticSemaKinds.td
  clang/lib/CodeGen/CGBuiltin.cpp
  clang/lib/Sema/SemaChecking.cpp
  clang/test/CodeGen/builtin-expect-with-probability-switch.c
  clang/test/CodeGen/builtin-expect-with-probability-template.cpp
  clang/test/CodeGen/builtin-expect-with-probability.c
  llvm/docs/BranchWeightMetadata.rst
  llvm/include/llvm/IR/Intrinsics.td
  llvm/lib/Transforms/Scalar/LowerExpectIntrinsic.cpp

Index: llvm/lib/Transforms/Scalar/LowerExpectIntrinsic.cpp
===
--- llvm/lib/Transforms/Scalar/LowerExpectIntrinsic.cpp
+++ llvm/lib/Transforms/Scalar/LowerExpectIntrinsic.cpp
@@ -55,13 +55,32 @@
 "unlikely-branch-weight", cl::Hidden, cl::init(1),
 cl::desc("Weight of the branch unlikely to be taken (default = 1)"));
 
+std::pair setBranchWeight(Intrinsic::ID IntrinsicID,
+  CallInst *CI, int BranchCount) {
+  if (IntrinsicID == Intrinsic::expect) {
+// __builtin_expect
+return {LikelyBranchWeight, UnlikelyBranchWeight};
+  } else {
+// __builtin_expect_with_probability
+assert(CI->getNumOperands() >= 3 &&
+   "expect with probability must have 3 arguments");
+ConstantFP *Confidence = dyn_cast(CI->getArgOperand(2));
+double TrueProb = Confidence->getValueAPF().convertToDouble();
+double FalseProb = (1.0 - TrueProb) / (BranchCount - 1);
+uint32_t LikelyBW = ceil((TrueProb * (double)(INT32_MAX - 1)) + 1.0);
+uint32_t UnlikelyBW = ceil((FalseProb * (double)(INT32_MAX - 1)) + 1.0);
+return {LikelyBW, UnlikelyBW};
+  }
+}
+
 static bool handleSwitchExpect(SwitchInst &SI) {
   CallInst *CI = dyn_cast(SI.getCondition());
   if (!CI)
 return false;
 
   Function *Fn = CI->getCalledFunction();
-  if (!Fn || Fn->getIntrinsicID() != Intrinsic::expect)
+  if (!Fn || (Fn->getIntrinsicID() != Intrinsic::expect &&
+  Fn->getIntrinsicID() != Intrinsic::expect_with_probability))
 return false;
 
   Value *ArgValue = CI->getArgOperand(0);
@@ -71,15 +90,20 @@
 
   SwitchInst::CaseHandle Case = *SI.findCaseValue(ExpectedValue);
   unsigned n = SI.getNumCases(); // +1 for default case.
-  SmallVector Weights(n + 1, UnlikelyBranchWeight);
+  std::pair WeightNums =
+  setBranchWeight(Fn->getIntrinsicID(), CI, n + 1);
+  uint32_t LikelyBranchWeightVal = WeightNums.first;
+  uint32_t UnlikelyBranchWeightVal = WeightNums.second;
+
+  SmallVector Weights(n + 1, UnlikelyBranchWeightVal);
 
   uint64_t Index = (Case == *SI.case_default()) ? 0 : Case.getCaseIndex() + 1;
-  Weights[Index] = LikelyBranchWeight;
+  Weights[Index] = LikelyBranchWeightVal;
 
-  SI.setMetadata(
-  LLVMContext::MD_misexpect,
-  MDBuilder(CI->getContext())
-  .createMisExpect(Index, LikelyBranchWeight, UnlikelyBranchWeight));
+  SI.setMetadata(LLVMContext::MD_misexpect,
+ MDBuilder(CI->getContext())
+ .createMisExpect(Index, LikelyBranchWeightVal,
+  UnlikelyBranchWeightVal));
 
   SI.setCondition(ArgValue);
   misexpect::checkFrontendInstrumentation(SI);
@@ -223,15 +247,19 @@
 return true;
   return false;
 };
+std::pair WeightNums = setBranchWeight(
+Expect->getCalledFunction()->getIntrinsicID(), Expect, 2);
+uint32_t LikelyBranchWeightVal = WeightNums.first;
+uint32_t UnlikelyBranchWeightVal = WeightNums.second;
 
 if (IsOpndComingFromSuccessor(BI->getSuccessor(1)))
-  BI->setMetadata(
-  LLVMContext::MD_prof,
-  MDB.createBranchWeights(LikelyBranchWeight, UnlikelyBranchWeight));
+  BI->setMetadata(LLVMContext::MD_prof,
+  MDB.createBranchWeights(LikelyBranchWeightVal,
+  UnlikelyBranchWeightVal));
 else if (IsOpndComingFromSuccessor(BI->getSuccessor(0)))
-  BI->setMetadata(
-  LLVMContext::MD_prof,
-  MDB.createBranchWeights(UnlikelyBranchWeight, LikelyBranchWeight));
+  BI->setMetadata(LLVMContext::MD_prof,
+  MDB.createBranchWeights(UnlikelyBranchWeightVal,
+  LikelyBranchWeightVal));
   }
 }
 
@@ -277,7 +305,8 @@
   }
 
   Function *Fn = CI->getCalledFunction();
-  if (!Fn || Fn->getIntrinsicID() != Intrinsic::expect)
+  if (!Fn || (Fn->getIntrinsicID() != Intrinsic::expect &&
+  Fn->getIntrinsicID() != Intrinsic::expect_with_probability))
 return false;
 
   Value *ArgValue = CI->getArgOperand(0);
@@ -289,13 +318,22 @@
   MDNode *Node;
   MD

[PATCH] D79830: Add support of __builtin_expect_with_probability

[Zhi Zhuang via Phabricator via cfe-commits]

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.


Thank you so much for the help. I just submitted a new patch.


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

https://reviews.llvm.org/D79830



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D79830: Add support of __builtin_expect_with_probability

[Zhi Zhuang via Phabricator via cfe-commits]

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 mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D79830: Add support of __builtin_expect_with_probability

[Zhi Zhuang via Phabricator via cfe-commits]

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:
  clang/include/clang/Basic/Builtins.def
  clang/include/clang/Basic/DiagnosticSemaKinds.td
  clang/lib/CodeGen/CGBuiltin.cpp
  clang/lib/Sema/SemaChecking.cpp
  clang/test/CodeGen/builtin-expect-with-probability-switch.c
  clang/test/CodeGen/builtin-expect-with-probability-template.cpp
  clang/test/CodeGen/builtin-expect-with-probability.c
  clang/test/Sema/builtin-expect-with-probability.cpp
  llvm/docs/BranchWeightMetadata.rst
  llvm/include/llvm/IR/Intrinsics.td
  llvm/lib/Transforms/Scalar/LowerExpectIntrinsic.cpp

Index: llvm/lib/Transforms/Scalar/LowerExpectIntrinsic.cpp
===
--- llvm/lib/Transforms/Scalar/LowerExpectIntrinsic.cpp
+++ llvm/lib/Transforms/Scalar/LowerExpectIntrinsic.cpp
@@ -55,13 +55,32 @@
 "unlikely-branch-weight", cl::Hidden, cl::init(1),
 cl::desc("Weight of the branch unlikely to be taken (default = 1)"));
 
+std::pair setBranchWeight(Intrinsic::ID IntrinsicID,
+  CallInst *CI, int BranchCount) {
+  if (IntrinsicID == Intrinsic::expect) {
+// __builtin_expect
+return {LikelyBranchWeight, UnlikelyBranchWeight};
+  } else {
+// __builtin_expect_with_probability
+assert(CI->getNumOperands() >= 3 &&
+   "expect with probability must have 3 arguments");
+ConstantFP *Confidence = dyn_cast(CI->getArgOperand(2));
+double TrueProb = Confidence->getValueAPF().convertToDouble();
+double FalseProb = (1.0 - TrueProb) / (BranchCount - 1);
+uint32_t LikelyBW = ceil((TrueProb * (double)(INT32_MAX - 1)) + 1.0);
+uint32_t UnlikelyBW = ceil((FalseProb * (double)(INT32_MAX - 1)) + 1.0);
+return {LikelyBW, UnlikelyBW};
+  }
+}
+
 static bool handleSwitchExpect(SwitchInst &SI) {
   CallInst *CI = dyn_cast(SI.getCondition());
   if (!CI)
 return false;
 
   Function *Fn = CI->getCalledFunction();
-  if (!Fn || Fn->getIntrinsicID() != Intrinsic::expect)
+  if (!Fn || (Fn->getIntrinsicID() != Intrinsic::expect &&
+  Fn->getIntrinsicID() != Intrinsic::expect_with_probability))
 return false;
 
   Value *ArgValue = CI->getArgOperand(0);
@@ -71,15 +90,20 @@
 
   SwitchInst::CaseHandle Case = *SI.findCaseValue(ExpectedValue);
   unsigned n = SI.getNumCases(); // +1 for default case.
-  SmallVector Weights(n + 1, UnlikelyBranchWeight);
+  std::pair WeightNums =
+  setBranchWeight(Fn->getIntrinsicID(), CI, n + 1);
+  uint32_t LikelyBranchWeightVal = WeightNums.first;
+  uint32_t UnlikelyBranchWeightVal = WeightNums.second;
+
+  SmallVector Weights(n + 1, UnlikelyBranchWeightVal);
 
   uint64_t Index = (Case == *SI.case_default()) ? 0 : Case.getCaseIndex() + 1;
-  Weights[Index] = LikelyBranchWeight;
+  Weights[Index] = LikelyBranchWeightVal;
 
-  SI.setMetadata(
-  LLVMContext::MD_misexpect,
-  MDBuilder(CI->getContext())
-  .createMisExpect(Index, LikelyBranchWeight, UnlikelyBranchWeight));
+  SI.setMetadata(LLVMContext::MD_misexpect,
+ MDBuilder(CI->getContext())
+ .createMisExpect(Index, LikelyBranchWeightVal,
+  UnlikelyBranchWeightVal));
 
   SI.setCondition(ArgValue);
   misexpect::checkFrontendInstrumentation(SI);
@@ -223,15 +247,19 @@
 return true;
   return false;
 };
+std::pair WeightNums = setBranchWeight(
+Expect->getCalledFunction()->getIntrinsicID(), Expect, 2);
+uint32_t LikelyBranchWeightVal = WeightNums.first;
+uint32_t UnlikelyBranchWeightVal = WeightNums.second;
 
 if (IsOpndComingFromSuccessor(BI->getSuccessor(1)))
-  BI->setMetadata(
-  LLVMContext::MD_prof,
-  MDB.createBranchWeights(LikelyBranchWeight, UnlikelyBranchWeight));
+  BI->setMetadata(LLVMContext::MD_prof,
+  MDB.createBranchWeights(LikelyBranchWeightVal,
+  UnlikelyBranchWeightVal));
 else if (IsOpndComingFromSuccessor(BI->getSuccessor(0)))
-  BI->setMetadata(
-  LLVMContext::MD_prof,
-  MDB.createBranchWeights(UnlikelyBranchWeight, LikelyBranchWeight));
+  BI->setMetadata(LLVMContext::MD_prof,
+  MDB.createBranchWeights(UnlikelyBranchWeightVal,
+  LikelyBranchWeightVal));
   }
 }
 
@@ -277,7 +305,8 @@
   }
 
   Function *Fn = CI->getCalledFunction();
-  if (!Fn || Fn->getIntrinsicID() != Intrinsic::expect)
+  if (!Fn || (Fn->getIntrinsicID() != Intrinsic::expect &&
+  Fn->getIntrinsicID() != Intrinsic::expect_with_probability))
 return false;
 
   Value *ArgValue = CI->getArgOperand(0);
@@ -289,13 +318,22 @@
   MDNode *Node;
   MDNode *ExpNode;
 
+  std::pair Weigh

[PATCH] D79830: Add support of __builtin_expect_with_probability

[Zhi Zhuang via Phabricator via cfe-commits]

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
  https://reviews.llvm.org/D79830/new/

https://reviews.llvm.org/D79830

Files:
  clang/include/clang/Basic/Builtins.def
  clang/include/clang/Basic/DiagnosticSemaKinds.td
  clang/lib/CodeGen/CGBuiltin.cpp
  clang/lib/Sema/SemaChecking.cpp
  clang/test/CodeGen/builtin-expect-with-probability-switch.c
  clang/test/CodeGen/builtin-expect-with-probability-template.cpp
  clang/test/CodeGen/builtin-expect-with-probability.c
  clang/test/Sema/builtin-expect-with-probability.cpp
  llvm/docs/BranchWeightMetadata.rst
  llvm/include/llvm/IR/Intrinsics.td
  llvm/lib/Transforms/Scalar/LowerExpectIntrinsic.cpp

Index: llvm/lib/Transforms/Scalar/LowerExpectIntrinsic.cpp
===
--- llvm/lib/Transforms/Scalar/LowerExpectIntrinsic.cpp
+++ llvm/lib/Transforms/Scalar/LowerExpectIntrinsic.cpp
@@ -55,13 +55,32 @@
 "unlikely-branch-weight", cl::Hidden, cl::init(1),
 cl::desc("Weight of the branch unlikely to be taken (default = 1)"));
 
+std::pair setBranchWeight(Intrinsic::ID IntrinsicID,
+  CallInst *CI, int BranchCount) {
+  if (IntrinsicID == Intrinsic::expect) {
+// __builtin_expect
+return {LikelyBranchWeight, UnlikelyBranchWeight};
+  } else {
+// __builtin_expect_with_probability
+assert(CI->getNumOperands() >= 3 &&
+   "expect with probability must have 3 arguments");
+ConstantFP *Confidence = dyn_cast(CI->getArgOperand(2));
+double TrueProb = Confidence->getValueAPF().convertToDouble();
+double FalseProb = (1.0 - TrueProb) / (BranchCount - 1);
+uint32_t LikelyBW = ceil((TrueProb * (double)(INT32_MAX - 1)) + 1.0);
+uint32_t UnlikelyBW = ceil((FalseProb * (double)(INT32_MAX - 1)) + 1.0);
+return {LikelyBW, UnlikelyBW};
+  }
+}
+
 static bool handleSwitchExpect(SwitchInst &SI) {
   CallInst *CI = dyn_cast(SI.getCondition());
   if (!CI)
 return false;
 
   Function *Fn = CI->getCalledFunction();
-  if (!Fn || Fn->getIntrinsicID() != Intrinsic::expect)
+  if (!Fn || (Fn->getIntrinsicID() != Intrinsic::expect &&
+  Fn->getIntrinsicID() != Intrinsic::expect_with_probability))
 return false;
 
   Value *ArgValue = CI->getArgOperand(0);
@@ -71,15 +90,20 @@
 
   SwitchInst::CaseHandle Case = *SI.findCaseValue(ExpectedValue);
   unsigned n = SI.getNumCases(); // +1 for default case.
-  SmallVector Weights(n + 1, UnlikelyBranchWeight);
+  std::pair WeightNums =
+  setBranchWeight(Fn->getIntrinsicID(), CI, n + 1);
+  uint32_t LikelyBranchWeightVal = WeightNums.first;
+  uint32_t UnlikelyBranchWeightVal = WeightNums.second;
+
+  SmallVector Weights(n + 1, UnlikelyBranchWeightVal);
 
   uint64_t Index = (Case == *SI.case_default()) ? 0 : Case.getCaseIndex() + 1;
-  Weights[Index] = LikelyBranchWeight;
+  Weights[Index] = LikelyBranchWeightVal;
 
-  SI.setMetadata(
-  LLVMContext::MD_misexpect,
-  MDBuilder(CI->getContext())
-  .createMisExpect(Index, LikelyBranchWeight, UnlikelyBranchWeight));
+  SI.setMetadata(LLVMContext::MD_misexpect,
+ MDBuilder(CI->getContext())
+ .createMisExpect(Index, LikelyBranchWeightVal,
+  UnlikelyBranchWeightVal));
 
   SI.setCondition(ArgValue);
   misexpect::checkFrontendInstrumentation(SI);
@@ -223,15 +247,19 @@
 return true;
   return false;
 };
+std::pair WeightNums = setBranchWeight(
+Expect->getCalledFunction()->getIntrinsicID(), Expect, 2);
+uint32_t LikelyBranchWeightVal = WeightNums.first;
+uint32_t UnlikelyBranchWeightVal = WeightNums.second;
 
 if (IsOpndComingFromSuccessor(BI->getSuccessor(1)))
-  BI->setMetadata(
-  LLVMContext::MD_prof,
-  MDB.createBranchWeights(LikelyBranchWeight, UnlikelyBranchWeight));
+  BI->setMetadata(LLVMContext::MD_prof,
+  MDB.createBranchWeights(LikelyBranchWeightVal,
+  UnlikelyBranchWeightVal));
 else if (IsOpndComingFromSuccessor(BI->getSuccessor(0)))
-  BI->setMetadata(
-  LLVMContext::MD_prof,
-  MDB.createBranchWeights(UnlikelyBranchWeight, LikelyBranchWeight));
+  BI->setMetadata(LLVMContext::MD_prof,
+  MDB.createBranchWeights(UnlikelyBranchWeightVal,
+  LikelyBranchWeightVal));
   }
 }
 
@@ -277,7 +305,8 @@
   }
 
   Function *Fn = CI->getCalledFunction();
-  if (!Fn || Fn->getIntrinsicID() != Intrinsic::expect)
+  if (!Fn || (Fn->getIntrinsicID() != Intrinsic::expect &&
+  Fn->getIntrinsicID() != Intrinsic::expect_with_probability))
 return false;
 
   Value *ArgValue = CI

[PATCH] D79830: Add support of __builtin_expect_with_probability

[Zhi Zhuang via Phabricator via cfe-commits]

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
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D79830: Add support of __builtin_expect_with_probability

[Zhi Zhuang via Phabricator via cfe-commits]

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
  clang/include/clang/Basic/DiagnosticSemaKinds.td
  clang/lib/CodeGen/CGBuiltin.cpp
  clang/lib/Sema/SemaChecking.cpp
  clang/test/CodeGen/builtin-expect-with-probability-switch.c
  clang/test/CodeGen/builtin-expect-with-probability-template.cpp
  clang/test/CodeGen/builtin-expect-with-probability.c
  clang/test/Sema/builtin-expect-with-probability.cpp
  llvm/docs/BranchWeightMetadata.rst
  llvm/include/llvm/IR/Intrinsics.td
  llvm/lib/Transforms/Scalar/LowerExpectIntrinsic.cpp

Index: llvm/lib/Transforms/Scalar/LowerExpectIntrinsic.cpp
===
--- llvm/lib/Transforms/Scalar/LowerExpectIntrinsic.cpp
+++ llvm/lib/Transforms/Scalar/LowerExpectIntrinsic.cpp
@@ -55,13 +55,32 @@
 "unlikely-branch-weight", cl::Hidden, cl::init(1),
 cl::desc("Weight of the branch unlikely to be taken (default = 1)"));
 
+std::pair setBranchWeight(Intrinsic::ID IntrinsicID,
+  CallInst *CI, int BranchCount) {
+  if (IntrinsicID == Intrinsic::expect) {
+// __builtin_expect
+return {LikelyBranchWeight, UnlikelyBranchWeight};
+  } else {
+// __builtin_expect_with_probability
+assert(CI->getNumOperands() >= 3 &&
+   "expect with probability must have 3 arguments");
+ConstantFP *Confidence = dyn_cast(CI->getArgOperand(2));
+double TrueProb = Confidence->getValueAPF().convertToDouble();
+double FalseProb = (1.0 - TrueProb) / (BranchCount - 1);
+uint32_t LikelyBW = ceil((TrueProb * (double)(INT32_MAX - 1)) + 1.0);
+uint32_t UnlikelyBW = ceil((FalseProb * (double)(INT32_MAX - 1)) + 1.0);
+return {LikelyBW, UnlikelyBW};
+  }
+}
+
 static bool handleSwitchExpect(SwitchInst &SI) {
   CallInst *CI = dyn_cast(SI.getCondition());
   if (!CI)
 return false;
 
   Function *Fn = CI->getCalledFunction();
-  if (!Fn || Fn->getIntrinsicID() != Intrinsic::expect)
+  if (!Fn || (Fn->getIntrinsicID() != Intrinsic::expect &&
+  Fn->getIntrinsicID() != Intrinsic::expect_with_probability))
 return false;
 
   Value *ArgValue = CI->getArgOperand(0);
@@ -71,15 +90,20 @@
 
   SwitchInst::CaseHandle Case = *SI.findCaseValue(ExpectedValue);
   unsigned n = SI.getNumCases(); // +1 for default case.
-  SmallVector Weights(n + 1, UnlikelyBranchWeight);
+  std::pair WeightNums =
+  setBranchWeight(Fn->getIntrinsicID(), CI, n + 1);
+  uint32_t LikelyBranchWeightVal = WeightNums.first;
+  uint32_t UnlikelyBranchWeightVal = WeightNums.second;
+
+  SmallVector Weights(n + 1, UnlikelyBranchWeightVal);
 
   uint64_t Index = (Case == *SI.case_default()) ? 0 : Case.getCaseIndex() + 1;
-  Weights[Index] = LikelyBranchWeight;
+  Weights[Index] = LikelyBranchWeightVal;
 
-  SI.setMetadata(
-  LLVMContext::MD_misexpect,
-  MDBuilder(CI->getContext())
-  .createMisExpect(Index, LikelyBranchWeight, UnlikelyBranchWeight));
+  SI.setMetadata(LLVMContext::MD_misexpect,
+ MDBuilder(CI->getContext())
+ .createMisExpect(Index, LikelyBranchWeightVal,
+  UnlikelyBranchWeightVal));
 
   SI.setCondition(ArgValue);
   misexpect::checkFrontendInstrumentation(SI);
@@ -223,15 +247,19 @@
 return true;
   return false;
 };
+std::pair WeightNums = setBranchWeight(
+Expect->getCalledFunction()->getIntrinsicID(), Expect, 2);
+uint32_t LikelyBranchWeightVal = WeightNums.first;
+uint32_t UnlikelyBranchWeightVal = WeightNums.second;
 
 if (IsOpndComingFromSuccessor(BI->getSuccessor(1)))
-  BI->setMetadata(
-  LLVMContext::MD_prof,
-  MDB.createBranchWeights(LikelyBranchWeight, UnlikelyBranchWeight));
+  BI->setMetadata(LLVMContext::MD_prof,
+  MDB.createBranchWeights(LikelyBranchWeightVal,
+  UnlikelyBranchWeightVal));
 else if (IsOpndComingFromSuccessor(BI->getSuccessor(0)))
-  BI->setMetadata(
-  LLVMContext::MD_prof,
-  MDB.createBranchWeights(UnlikelyBranchWeight, LikelyBranchWeight));
+  BI->setMetadata(LLVMContext::MD_prof,
+  MDB.createBranchWeights(UnlikelyBranchWeightVal,
+  LikelyBranchWeightVal));
   }
 }
 
@@ -277,7 +305,8 @@
   }
 
   Function *Fn = CI->getCalledFunction();
-  if (!Fn || Fn->getIntrinsicID() != Intrinsic::expect)
+  if (!Fn || (Fn->getIntrinsicID() != Intrinsic::expect &&
+  Fn->getIntrinsicID() != Intrinsic::expect_with_probability))
 return false;
 
   Value *ArgValue = CI->getArgOperand(0);
@@ -289,13 +318,22 @@
   MDNode *Node;
   MDNode *ExpNode;
 
+  std::pair WeightNums =
+

[PATCH] D79830: Add support of __builtin_expect_with_probability

[Zhi Zhuang via Phabricator via cfe-commits]

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 <= llvm::APFloat(1.0))) {
+  Diag(ProbArg->getLocStart(), diag::err_probability_out_of_range)

lebedev.ri wrote:
> This doesn't look right to me, but maybe i'm misreading something?
> Assuming the `!` is intentional to avoid some fp weirdness (if not, simplify 
> this)
> shouldn't this be an `&&`, not `||`?
> 
> Consider:
> `Probability = -1`
> ```
> 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?
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.



Comment at: clang/test/Sema/builtin-expect-with-probability.cpp:19-20
+  dummy = __builtin_expect_with_probability(x > 0, 1, 0.9);
+  dummy = __builtin_expect_with_probability(x > 0, 1, 1.1); // expected-error 
{{probability of __builtin_expect_with_probability is outside the range [0.0, 
1.0]}}
+  dummy = __builtin_expect_with_probability(x > 0, 1, -1); // expected-error 
{{probability of __builtin_expect_with_probability is outside the range [0.0, 
1.0]}}
+  dummy = __builtin_expect_with_probability(x > 0, 1, p); // expected-error 
{{probability of __builtin_expect_with_probability must be constant 
floating-point expression}} expected-note {{read of non-constexpr variable 'p' 
is not allowed in a constant expression}}

lebedev.ri wrote:
> Do these tests actually pass?
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!


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

https://reviews.llvm.org/D79830



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D79830: Add support of __builtin_expect_with_probability

[Zhi Zhuang via Phabricator via cfe-commits]

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(), diag::err_probability_out_of_range)

rsmith wrote:
> 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`.
Hi, thank you for comments! I have several questions here:
(1) I do not understand why "APFloat Probability >= APFloat(0.0)" is affected 
by target here. Is this just a double come from C front-end and I check it 
here? 
(2) And also not sure in CGBuiltin, is there I convert it to llvm intrinsic? 
How does target double type affect it?
(3) If I convert APFloat to IEEEDouble, should I use APF.convertToDouble or 
create a DoubleAPFloat. My original code did APFloat.convertToDouble and 
compare, and I do not sure about the difference.



Comment at: llvm/docs/BranchWeightMetadata.rst:128-135
+Built-in ``expect.with.probability`` Instruction
+
+
+``__builtin_expect_with_probability(long exp, long c, double probability)`` has
+the same semantics as ``__builtin_expect``, but the caller provides the
+probability that ``exp == c``. The last argument ``probability`` must be
+constant floating-point expression and be in the range [0.0, 1.0] inclusive.

rsmith wrote:
> 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:
> 
>  * Move all of that documentation into the Clang user's manual, as a 
> description of the `__builtin_expect*` builtin functions.
>  * Here in the LLVM documentation, add documentation for the `llvm.expect` 
> and `llvm.expect.with.probability` intrinsic functions, and include a note 
> pointing to the Clang documentation and suggesting that `__builtin_expect` 
> and `__builtin_expect_with_probability` can be used to generate these 
> intrinsics in C and C++ source files.
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.


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

https://reviews.llvm.org/D79830



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D79830: Add support of __builtin_expect_with_probability

[Zhi Zhuang via Phabricator via cfe-commits]

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
+llvm::APFloat Probability = Eval.Val.getFloat();
+if (!(Probability >= llvm::APFloat(0.0) &&
+  Probability <= llvm::APFloat(1.0))) {
+  Diag(ProbArg->getLocStart(), diag::err_probability_out_of_range)

LukeZhuang wrote:
> rsmith wrote:
> > 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`.
> Hi, thank you for comments! I have several questions here:
> (1) I do not understand why "APFloat Probability >= APFloat(0.0)" is affected 
> by target here. Is this just a double come from C front-end and I check it 
> here? 
> (2) And also not sure in CGBuiltin, is there I convert it to llvm intrinsic? 
> How does target double type affect it?
> (3) If I convert APFloat to IEEEDouble, should I use APF.convertToDouble or 
> create a DoubleAPFloat. My original code did APFloat.convertToDouble and 
> compare, and I do not sure about the difference.
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.



Comment at: clang/test/Sema/builtin-expect-with-probability.cpp:18
+  bool dummy;
+  dummy = __builtin_expect_with_probability(x > 0, 1, 0.9);
+  dummy = __builtin_expect_with_probability(x > 0, 1, 1.1); // expected-error 
{{probability of __builtin_expect_with_probability is outside the range [0.0, 
1.0]}}

rsmith wrote:
> 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.
added more edge cases here and example for AVR target



Comment at: llvm/docs/BranchWeightMetadata.rst:18
 Branch weights might be fetch from the profiling file, or generated based on
-`__builtin_expect`_ instruction.
+`__builtin_expect`_ and `__builtin_expect_with_probability`_ instruction.
 

rsmith wrote:
> 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.
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.


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

https://reviews.llvm.org/D79830



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D79830: Add support of __builtin_expect_with_probability

[Zhi Zhuang via Phabricator via cfe-commits]

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
  https://reviews.llvm.org/D79830/new/

https://reviews.llvm.org/D79830

Files:
  clang/include/clang/Basic/Builtins.def
  clang/include/clang/Basic/DiagnosticSemaKinds.td
  clang/lib/CodeGen/CGBuiltin.cpp
  clang/lib/Sema/SemaChecking.cpp
  clang/test/CodeGen/builtin-expect-with-probability-template.cpp
  clang/test/CodeGen/builtin-expect-with-probability.c
  clang/test/Sema/builtin-expect-with-probability-avr.cpp
  clang/test/Sema/builtin-expect-with-probability.cpp
  llvm/docs/BranchWeightMetadata.rst
  llvm/include/llvm/IR/Intrinsics.td
  llvm/lib/Transforms/Scalar/LowerExpectIntrinsic.cpp
  llvm/test/Transforms/LowerExpectIntrinsic/basic.ll

Index: llvm/test/Transforms/LowerExpectIntrinsic/basic.ll
===
--- llvm/test/Transforms/LowerExpectIntrinsic/basic.ll
+++ llvm/test/Transforms/LowerExpectIntrinsic/basic.ll
@@ -285,6 +285,29 @@
 
 declare i1 @llvm.expect.i1(i1, i1) nounwind readnone
 
+; CHECK-LABEL: @test11(
+define dso_local void @test11(i32 %0) noinline nounwind optnone uwtable {
+  %2 = alloca i32, align 4
+  store i32 %0, i32* %2, align 4
+  %3 = load i32, i32* %2, align 4
+  %4 = icmp sgt i32 %3, 0
+  %5 = zext i1 %4 to i32
+  %6 = sext i32 %5 to i64
+  %7 = call i64 @llvm.expect.with.probability.i64(i64 %6, i64 1, double 8.00e-01)
+  %8 = icmp ne i64 %7, 0
+  br i1 %8, label %9, label %10
+
+9:; preds = %1
+  call void (...) @ff()
+  br label %10
+
+10:   ; preds = %9, %1
+  ret void
+}
+
+declare i64 @llvm.expect.with.probability.i64(i64, i64, double) nounwind readnone willreturn
+declare dso_local void @ff(...)
+
 ; CHECK: !0 = !{!"branch_weights", i32 2000, i32 1}
 ; CHECK: !1 = !{!"misexpect", i64 0, i64 2000, i64 1}
 ; CHECK: !2 = !{!"branch_weights", i32 1, i32 2000}
@@ -292,3 +315,5 @@
 ; CHECK: !4 = !{!"branch_weights", i32 1, i32 1, i32 2000}
 ; CHECK: !5 = !{!"misexpect", i64 2, i64 2000, i64 1}
 ; CHECK: !6 = !{!"branch_weights", i32 2000, i32 1, i32 1}
+; CHECK: !7 = !{!"branch_weights", i32 1717986918, i32 429496731}
+; CHECK: !8 = !{!"misexpect", i64 0, i64 1717986918, i64 429496731}
Index: llvm/lib/Transforms/Scalar/LowerExpectIntrinsic.cpp
===
--- llvm/lib/Transforms/Scalar/LowerExpectIntrinsic.cpp
+++ llvm/lib/Transforms/Scalar/LowerExpectIntrinsic.cpp
@@ -55,13 +55,32 @@
 "unlikely-branch-weight", cl::Hidden, cl::init(1),
 cl::desc("Weight of the branch unlikely to be taken (default = 1)"));
 
+std::pair setBranchWeight(Intrinsic::ID IntrinsicID,
+  CallInst *CI, int BranchCount) {
+  if (IntrinsicID == Intrinsic::expect) {
+// __builtin_expect
+return {LikelyBranchWeight, UnlikelyBranchWeight};
+  } else {
+// __builtin_expect_with_probability
+assert(CI->getNumOperands() >= 3 &&
+   "expect with probability must have 3 arguments");
+ConstantFP *Confidence = dyn_cast(CI->getArgOperand(2));
+double TrueProb = Confidence->getValueAPF().convertToDouble();
+double FalseProb = (1.0 - TrueProb) / (BranchCount - 1);
+uint32_t LikelyBW = ceil((TrueProb * (double)(INT32_MAX - 1)) + 1.0);
+uint32_t UnlikelyBW = ceil((FalseProb * (double)(INT32_MAX - 1)) + 1.0);
+return {LikelyBW, UnlikelyBW};
+  }
+}
+
 static bool handleSwitchExpect(SwitchInst &SI) {
   CallInst *CI = dyn_cast(SI.getCondition());
   if (!CI)
 return false;
 
   Function *Fn = CI->getCalledFunction();
-  if (!Fn || Fn->getIntrinsicID() != Intrinsic::expect)
+  if (!Fn || (Fn->getIntrinsicID() != Intrinsic::expect &&
+  Fn->getIntrinsicID() != Intrinsic::expect_with_probability))
 return false;
 
   Value *ArgValue = CI->getArgOperand(0);
@@ -71,15 +90,20 @@
 
   SwitchInst::CaseHandle Case = *SI.findCaseValue(ExpectedValue);
   unsigned n = SI.getNumCases(); // +1 for default case.
-  SmallVector Weights(n + 1, UnlikelyBranchWeight);
+  std::pair WeightNums =
+  setBranchWeight(Fn->getIntrinsicID(), CI, n + 1);
+  uint32_t LikelyBranchWeightVal = WeightNums.first;
+  uint32_t UnlikelyBranchWeightVal = WeightNums.second;
+
+  SmallVector Weights(n + 1, UnlikelyBranchWeightVal);
 
   uint64_t Index = (Case == *SI.case_default()) ? 0 : Case.getCaseIndex() + 1;
-  Weights[Index] = LikelyBranchWeight;
+  Weights[Index] = LikelyBranchWeightVal;
 
-  SI.setMetadata(
-  LLVMContext::MD_misexpect,
-  MDBuilder(CI->getContext())
-  .createMisExpect(Index, LikelyBranchWeight, UnlikelyBranchWeight));
+  SI.setMetadata(LLVMContext::MD_misexpect,
+ MDBuilder(CI->getCont

[PATCH] D79830: Add support of __builtin_expect_with_probability

[Zhi Zhuang via Phabricator via cfe-commits]

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/

https://reviews.llvm.org/D79830

Files:
  clang/include/clang/Basic/Builtins.def
  clang/include/clang/Basic/DiagnosticSemaKinds.td
  clang/lib/CodeGen/CGBuiltin.cpp
  clang/lib/Sema/SemaChecking.cpp
  clang/test/CodeGen/builtin-expect-with-probability-template.cpp
  clang/test/CodeGen/builtin-expect-with-probability.c
  clang/test/Sema/builtin-expect-with-probability-avr.cpp
  clang/test/Sema/builtin-expect-with-probability.cpp
  llvm/docs/BranchWeightMetadata.rst
  llvm/docs/LangRef.rst
  llvm/include/llvm/IR/Intrinsics.td
  llvm/lib/Transforms/Scalar/LowerExpectIntrinsic.cpp
  llvm/test/Transforms/LowerExpectIntrinsic/basic.ll

Index: llvm/test/Transforms/LowerExpectIntrinsic/basic.ll
===
--- llvm/test/Transforms/LowerExpectIntrinsic/basic.ll
+++ llvm/test/Transforms/LowerExpectIntrinsic/basic.ll
@@ -285,6 +285,29 @@
 
 declare i1 @llvm.expect.i1(i1, i1) nounwind readnone
 
+; CHECK-LABEL: @test11(
+define dso_local void @test11(i32 %0) noinline nounwind optnone uwtable {
+  %2 = alloca i32, align 4
+  store i32 %0, i32* %2, align 4
+  %3 = load i32, i32* %2, align 4
+  %4 = icmp sgt i32 %3, 0
+  %5 = zext i1 %4 to i32
+  %6 = sext i32 %5 to i64
+  %7 = call i64 @llvm.expect.with.probability.i64(i64 %6, i64 1, double 8.00e-01)
+  %8 = icmp ne i64 %7, 0
+  br i1 %8, label %9, label %10
+
+9:; preds = %1
+  call void (...) @ff()
+  br label %10
+
+10:   ; preds = %9, %1
+  ret void
+}
+
+declare i64 @llvm.expect.with.probability.i64(i64, i64, double) nounwind readnone willreturn
+declare dso_local void @ff(...)
+
 ; CHECK: !0 = !{!"branch_weights", i32 2000, i32 1}
 ; CHECK: !1 = !{!"misexpect", i64 0, i64 2000, i64 1}
 ; CHECK: !2 = !{!"branch_weights", i32 1, i32 2000}
@@ -292,3 +315,5 @@
 ; CHECK: !4 = !{!"branch_weights", i32 1, i32 1, i32 2000}
 ; CHECK: !5 = !{!"misexpect", i64 2, i64 2000, i64 1}
 ; CHECK: !6 = !{!"branch_weights", i32 2000, i32 1, i32 1}
+; CHECK: !7 = !{!"branch_weights", i32 1717986918, i32 429496731}
+; CHECK: !8 = !{!"misexpect", i64 0, i64 1717986918, i64 429496731}
Index: llvm/lib/Transforms/Scalar/LowerExpectIntrinsic.cpp
===
--- llvm/lib/Transforms/Scalar/LowerExpectIntrinsic.cpp
+++ llvm/lib/Transforms/Scalar/LowerExpectIntrinsic.cpp
@@ -55,13 +55,32 @@
 "unlikely-branch-weight", cl::Hidden, cl::init(1),
 cl::desc("Weight of the branch unlikely to be taken (default = 1)"));
 
+std::pair setBranchWeight(Intrinsic::ID IntrinsicID,
+  CallInst *CI, int BranchCount) {
+  if (IntrinsicID == Intrinsic::expect) {
+// __builtin_expect
+return {LikelyBranchWeight, UnlikelyBranchWeight};
+  } else {
+// __builtin_expect_with_probability
+assert(CI->getNumOperands() >= 3 &&
+   "expect with probability must have 3 arguments");
+ConstantFP *Confidence = dyn_cast(CI->getArgOperand(2));
+double TrueProb = Confidence->getValueAPF().convertToDouble();
+double FalseProb = (1.0 - TrueProb) / (BranchCount - 1);
+uint32_t LikelyBW = ceil((TrueProb * (double)(INT32_MAX - 1)) + 1.0);
+uint32_t UnlikelyBW = ceil((FalseProb * (double)(INT32_MAX - 1)) + 1.0);
+return {LikelyBW, UnlikelyBW};
+  }
+}
+
 static bool handleSwitchExpect(SwitchInst &SI) {
   CallInst *CI = dyn_cast(SI.getCondition());
   if (!CI)
 return false;
 
   Function *Fn = CI->getCalledFunction();
-  if (!Fn || Fn->getIntrinsicID() != Intrinsic::expect)
+  if (!Fn || (Fn->getIntrinsicID() != Intrinsic::expect &&
+  Fn->getIntrinsicID() != Intrinsic::expect_with_probability))
 return false;
 
   Value *ArgValue = CI->getArgOperand(0);
@@ -71,15 +90,20 @@
 
   SwitchInst::CaseHandle Case = *SI.findCaseValue(ExpectedValue);
   unsigned n = SI.getNumCases(); // +1 for default case.
-  SmallVector Weights(n + 1, UnlikelyBranchWeight);
+  std::pair WeightNums =
+  setBranchWeight(Fn->getIntrinsicID(), CI, n + 1);
+  uint32_t LikelyBranchWeightVal = WeightNums.first;
+  uint32_t UnlikelyBranchWeightVal = WeightNums.second;
+
+  SmallVector Weights(n + 1, UnlikelyBranchWeightVal);
 
   uint64_t Index = (Case == *SI.case_default()) ? 0 : Case.getCaseIndex() + 1;
-  Weights[Index] = LikelyBranchWeight;
+  Weights[Index] = LikelyBranchWeightVal;
 
-  SI.setMetadata(
-  LLVMContext::MD_misexpect,
-  MDBuilder(CI->getContext())
-  .createMisExpect(Index, LikelyBranchWeight, UnlikelyBranchWeight));
+  SI.setMetadata(LLVMContext::MD_misexpect,
+ MDBuilder(CI->getContext())
+   

[PATCH] D79830: Add support of __builtin_expect_with_probability

[Zhi Zhuang via Phabricator via cfe-commits]

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/

https://reviews.llvm.org/D79830

Files:
  clang/include/clang/Basic/Builtins.def
  clang/include/clang/Basic/DiagnosticSemaKinds.td
  clang/lib/CodeGen/CGBuiltin.cpp
  clang/lib/Sema/SemaChecking.cpp
  clang/test/CodeGen/builtin-expect-with-probability-template.cpp
  clang/test/CodeGen/builtin-expect-with-probability.c
  clang/test/Sema/builtin-expect-with-probability-avr.cpp
  clang/test/Sema/builtin-expect-with-probability.cpp
  llvm/docs/BranchWeightMetadata.rst
  llvm/docs/LangRef.rst
  llvm/include/llvm/IR/Intrinsics.td
  llvm/lib/Transforms/Scalar/LowerExpectIntrinsic.cpp
  llvm/test/Transforms/LowerExpectIntrinsic/expect-with-probability.ll

Index: llvm/test/Transforms/LowerExpectIntrinsic/expect-with-probability.ll
===
--- /dev/null
+++ llvm/test/Transforms/LowerExpectIntrinsic/expect-with-probability.ll
@@ -0,0 +1,295 @@
+; RUN: opt -lower-expect -strip-dead-prototypes -S -o - < %s | FileCheck %s
+; RUN: opt -S -passes='function(lower-expect),strip-dead-prototypes' < %s | FileCheck %s
+
+; CHECK-LABEL: @test1(
+define i32 @test1(i32 %x) nounwind uwtable ssp {
+entry:
+  %retval = alloca i32, align 4
+  %x.addr = alloca i32, align 4
+  store i32 %x, i32* %x.addr, align 4
+  %tmp = load i32, i32* %x.addr, align 4
+  %cmp = icmp sgt i32 %tmp, 1
+  %conv = zext i1 %cmp to i32
+  %conv1 = sext i32 %conv to i64
+  %expval = call i64 @llvm.expect.with.probability.i64(i64 %conv1, i64 1, double 8.00e-01)
+  %tobool = icmp ne i64 %expval, 0
+; CHECK: !prof !0, !misexpect !1
+; CHECK-NOT: @llvm.expect.with.probability
+  br i1 %tobool, label %if.then, label %if.end
+
+if.then:  ; preds = %entry
+  %call = call i32 (...) @f()
+  store i32 %call, i32* %retval
+  br label %return
+
+if.end:   ; preds = %entry
+  store i32 1, i32* %retval
+  br label %return
+
+return:   ; preds = %if.end, %if.then
+  %0 = load i32, i32* %retval
+  ret i32 %0
+}
+
+declare i64 @llvm.expect.with.probability.i64(i64, i64, double) nounwind readnone
+
+declare i32 @f(...)
+
+; CHECK-LABEL: @test2(
+define i32 @test2(i32 %x) nounwind uwtable ssp {
+entry:
+  %retval = alloca i32, align 4
+  %x.addr = alloca i32, align 4
+  store i32 %x, i32* %x.addr, align 4
+  %tmp = load i32, i32* %x.addr, align 4
+  %conv = sext i32 %tmp to i64
+  %expval = call i64 @llvm.expect.with.probability.i64(i64 %conv, i64 1, double 8.00e-01)
+  %tobool = icmp ne i64 %expval, 0
+; CHECK: !prof !0, !misexpect !1
+; CHECK-NOT: @llvm.expect.with.probability
+  br i1 %tobool, label %if.then, label %if.end
+
+if.then:  ; preds = %entry
+  %call = call i32 (...) @f()
+  store i32 %call, i32* %retval
+  br label %return
+
+if.end:   ; preds = %entry
+  store i32 1, i32* %retval
+  br label %return
+
+return:   ; preds = %if.end, %if.then
+  %0 = load i32, i32* %retval
+  ret i32 %0
+}
+
+; CHECK-LABEL: @test3(
+define i32 @test3(i32 %x) nounwind uwtable ssp {
+entry:
+  %retval = alloca i32, align 4
+  %x.addr = alloca i32, align 4
+  store i32 %x, i32* %x.addr, align 4
+  %tmp = load i32, i32* %x.addr, align 4
+  %tobool = icmp ne i32 %tmp, 0
+  %lnot = xor i1 %tobool, true
+  %lnot.ext = zext i1 %lnot to i32
+  %conv = sext i32 %lnot.ext to i64
+  %expval = call i64 @llvm.expect.with.probability.i64(i64 %conv, i64 1, double 8.00e-01)
+  %tobool1 = icmp ne i64 %expval, 0
+; CHECK: !prof !0, !misexpect !1
+; CHECK-NOT: @llvm.expect.with.probability
+  br i1 %tobool1, label %if.then, label %if.end
+
+if.then:  ; preds = %entry
+  %call = call i32 (...) @f()
+  store i32 %call, i32* %retval
+  br label %return
+
+if.end:   ; preds = %entry
+  store i32 1, i32* %retval
+  br label %return
+
+return:   ; preds = %if.end, %if.then
+  %0 = load i32, i32* %retval
+  ret i32 %0
+}
+
+; CHECK-LABEL: @test4(
+define i32 @test4(i32 %x) nounwind uwtable ssp {
+entry:
+  %retval = alloca i32, align 4
+  %x.addr = alloca i32, align 4
+  store i32 %x, i32* %x.addr, align 4
+  %tmp = load i32, i32* %x.addr, align 4
+  %tobool = icmp ne i32 %tmp, 0
+  %lnot = xor i1 %tobool, true
+  %lnot1 = xor i1 %lnot, true
+  %lnot.ext = zext i1 %lnot1 to i32
+  %conv = sext i32 %lnot.ext to i64
+  %expval = call i64 @llvm.expect.with.probability.i64(i64 %conv, i64 1, double 8.00e-01)
+  %tobool2 = icmp ne i64 %expval, 0
+; CHECK: !prof !0, !misexpect !1
+; CHECK-NOT: @llvm.expect.with.proba

[PATCH] D79830: Add support of __builtin_expect_with_probability

[Zhi Zhuang via Phabricator via cfe-commits]

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:
  clang/include/clang/Basic/Builtins.def
  clang/include/clang/Basic/DiagnosticSemaKinds.td
  clang/lib/CodeGen/CGBuiltin.cpp
  clang/lib/Sema/SemaChecking.cpp
  clang/test/CodeGen/builtin-expect-with-probability.cpp
  clang/test/Sema/builtin-expect-with-probability-avr.cpp
  clang/test/Sema/builtin-expect-with-probability.cpp
  llvm/docs/BranchWeightMetadata.rst
  llvm/docs/LangRef.rst
  llvm/include/llvm/IR/Intrinsics.td
  llvm/lib/Transforms/Scalar/LowerExpectIntrinsic.cpp
  llvm/test/Transforms/LowerExpectIntrinsic/expect-with-probability.ll

Index: llvm/test/Transforms/LowerExpectIntrinsic/expect-with-probability.ll
===
--- /dev/null
+++ llvm/test/Transforms/LowerExpectIntrinsic/expect-with-probability.ll
@@ -0,0 +1,295 @@
+; RUN: opt -lower-expect -strip-dead-prototypes -S -o - < %s | FileCheck %s
+; RUN: opt -S -passes='function(lower-expect),strip-dead-prototypes' < %s | FileCheck %s
+
+; CHECK-LABEL: @test1(
+define i32 @test1(i32 %x) nounwind uwtable ssp {
+entry:
+  %retval = alloca i32, align 4
+  %x.addr = alloca i32, align 4
+  store i32 %x, i32* %x.addr, align 4
+  %tmp = load i32, i32* %x.addr, align 4
+  %cmp = icmp sgt i32 %tmp, 1
+  %conv = zext i1 %cmp to i32
+  %conv1 = sext i32 %conv to i64
+  %expval = call i64 @llvm.expect.with.probability.i64(i64 %conv1, i64 1, double 8.00e-01)
+  %tobool = icmp ne i64 %expval, 0
+; CHECK: !prof !0, !misexpect !1
+; CHECK-NOT: @llvm.expect.with.probability
+  br i1 %tobool, label %if.then, label %if.end
+
+if.then:  ; preds = %entry
+  %call = call i32 (...) @f()
+  store i32 %call, i32* %retval
+  br label %return
+
+if.end:   ; preds = %entry
+  store i32 1, i32* %retval
+  br label %return
+
+return:   ; preds = %if.end, %if.then
+  %0 = load i32, i32* %retval
+  ret i32 %0
+}
+
+declare i64 @llvm.expect.with.probability.i64(i64, i64, double) nounwind readnone
+
+declare i32 @f(...)
+
+; CHECK-LABEL: @test2(
+define i32 @test2(i32 %x) nounwind uwtable ssp {
+entry:
+  %retval = alloca i32, align 4
+  %x.addr = alloca i32, align 4
+  store i32 %x, i32* %x.addr, align 4
+  %tmp = load i32, i32* %x.addr, align 4
+  %conv = sext i32 %tmp to i64
+  %expval = call i64 @llvm.expect.with.probability.i64(i64 %conv, i64 1, double 8.00e-01)
+  %tobool = icmp ne i64 %expval, 0
+; CHECK: !prof !0, !misexpect !1
+; CHECK-NOT: @llvm.expect.with.probability
+  br i1 %tobool, label %if.then, label %if.end
+
+if.then:  ; preds = %entry
+  %call = call i32 (...) @f()
+  store i32 %call, i32* %retval
+  br label %return
+
+if.end:   ; preds = %entry
+  store i32 1, i32* %retval
+  br label %return
+
+return:   ; preds = %if.end, %if.then
+  %0 = load i32, i32* %retval
+  ret i32 %0
+}
+
+; CHECK-LABEL: @test3(
+define i32 @test3(i32 %x) nounwind uwtable ssp {
+entry:
+  %retval = alloca i32, align 4
+  %x.addr = alloca i32, align 4
+  store i32 %x, i32* %x.addr, align 4
+  %tmp = load i32, i32* %x.addr, align 4
+  %tobool = icmp ne i32 %tmp, 0
+  %lnot = xor i1 %tobool, true
+  %lnot.ext = zext i1 %lnot to i32
+  %conv = sext i32 %lnot.ext to i64
+  %expval = call i64 @llvm.expect.with.probability.i64(i64 %conv, i64 1, double 8.00e-01)
+  %tobool1 = icmp ne i64 %expval, 0
+; CHECK: !prof !0, !misexpect !1
+; CHECK-NOT: @llvm.expect.with.probability
+  br i1 %tobool1, label %if.then, label %if.end
+
+if.then:  ; preds = %entry
+  %call = call i32 (...) @f()
+  store i32 %call, i32* %retval
+  br label %return
+
+if.end:   ; preds = %entry
+  store i32 1, i32* %retval
+  br label %return
+
+return:   ; preds = %if.end, %if.then
+  %0 = load i32, i32* %retval
+  ret i32 %0
+}
+
+; CHECK-LABEL: @test4(
+define i32 @test4(i32 %x) nounwind uwtable ssp {
+entry:
+  %retval = alloca i32, align 4
+  %x.addr = alloca i32, align 4
+  store i32 %x, i32* %x.addr, align 4
+  %tmp = load i32, i32* %x.addr, align 4
+  %tobool = icmp ne i32 %tmp, 0
+  %lnot = xor i1 %tobool, true
+  %lnot1 = xor i1 %lnot, true
+  %lnot.ext = zext i1 %lnot1 to i32
+  %conv = sext i32 %lnot.ext to i64
+  %expval = call i64 @llvm.expect.with.probability.i64(i64 %conv, i64 1, double 8.00e-01)
+  %tobool2 = icmp ne i64 %expval, 0
+; CHECK: !prof !0, !misexpect !1
+; CHECK-NOT: @llvm.expect.with.probability
+  br i1 %tobool2, label %if.then, label %if.end
+
+if.then:

[PATCH] D79830: Add support of __builtin_expect_with_probability

[Zhi Zhuang via Phabricator via cfe-commits]

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
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D79830: Add support of __builtin_expect_with_probability

[Zhi Zhuang via Phabricator via cfe-commits]

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



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D79830: Add support of __builtin_expect_with_probability

[Zhi Zhuang via Phabricator via cfe-commits]

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:
  clang/include/clang/Basic/Builtins.def
  clang/include/clang/Basic/DiagnosticSemaKinds.td
  clang/lib/CodeGen/CGBuiltin.cpp
  clang/lib/Sema/SemaChecking.cpp
  clang/test/CodeGen/builtin-expect-with-probability.cpp
  clang/test/Sema/builtin-expect-with-probability-avr.cpp
  clang/test/Sema/builtin-expect-with-probability.cpp
  llvm/docs/BranchWeightMetadata.rst
  llvm/docs/LangRef.rst
  llvm/include/llvm/IR/Intrinsics.td
  llvm/lib/Transforms/Scalar/LowerExpectIntrinsic.cpp
  llvm/test/Transforms/LowerExpectIntrinsic/expect-with-probability.ll

Index: llvm/test/Transforms/LowerExpectIntrinsic/expect-with-probability.ll
===
--- /dev/null
+++ llvm/test/Transforms/LowerExpectIntrinsic/expect-with-probability.ll
@@ -0,0 +1,295 @@
+; RUN: opt -lower-expect -strip-dead-prototypes -S -o - < %s | FileCheck %s
+; RUN: opt -S -passes='function(lower-expect),strip-dead-prototypes' < %s | FileCheck %s
+
+; CHECK-LABEL: @test1(
+define i32 @test1(i32 %x) nounwind uwtable ssp {
+entry:
+  %retval = alloca i32, align 4
+  %x.addr = alloca i32, align 4
+  store i32 %x, i32* %x.addr, align 4
+  %tmp = load i32, i32* %x.addr, align 4
+  %cmp = icmp sgt i32 %tmp, 1
+  %conv = zext i1 %cmp to i32
+  %conv1 = sext i32 %conv to i64
+  %expval = call i64 @llvm.expect.with.probability.i64(i64 %conv1, i64 1, double 8.00e-01)
+  %tobool = icmp ne i64 %expval, 0
+; CHECK: !prof !0, !misexpect !1
+; CHECK-NOT: @llvm.expect.with.probability
+  br i1 %tobool, label %if.then, label %if.end
+
+if.then:  ; preds = %entry
+  %call = call i32 (...) @f()
+  store i32 %call, i32* %retval
+  br label %return
+
+if.end:   ; preds = %entry
+  store i32 1, i32* %retval
+  br label %return
+
+return:   ; preds = %if.end, %if.then
+  %0 = load i32, i32* %retval
+  ret i32 %0
+}
+
+declare i64 @llvm.expect.with.probability.i64(i64, i64, double) nounwind readnone
+
+declare i32 @f(...)
+
+; CHECK-LABEL: @test2(
+define i32 @test2(i32 %x) nounwind uwtable ssp {
+entry:
+  %retval = alloca i32, align 4
+  %x.addr = alloca i32, align 4
+  store i32 %x, i32* %x.addr, align 4
+  %tmp = load i32, i32* %x.addr, align 4
+  %conv = sext i32 %tmp to i64
+  %expval = call i64 @llvm.expect.with.probability.i64(i64 %conv, i64 1, double 8.00e-01)
+  %tobool = icmp ne i64 %expval, 0
+; CHECK: !prof !0, !misexpect !1
+; CHECK-NOT: @llvm.expect.with.probability
+  br i1 %tobool, label %if.then, label %if.end
+
+if.then:  ; preds = %entry
+  %call = call i32 (...) @f()
+  store i32 %call, i32* %retval
+  br label %return
+
+if.end:   ; preds = %entry
+  store i32 1, i32* %retval
+  br label %return
+
+return:   ; preds = %if.end, %if.then
+  %0 = load i32, i32* %retval
+  ret i32 %0
+}
+
+; CHECK-LABEL: @test3(
+define i32 @test3(i32 %x) nounwind uwtable ssp {
+entry:
+  %retval = alloca i32, align 4
+  %x.addr = alloca i32, align 4
+  store i32 %x, i32* %x.addr, align 4
+  %tmp = load i32, i32* %x.addr, align 4
+  %tobool = icmp ne i32 %tmp, 0
+  %lnot = xor i1 %tobool, true
+  %lnot.ext = zext i1 %lnot to i32
+  %conv = sext i32 %lnot.ext to i64
+  %expval = call i64 @llvm.expect.with.probability.i64(i64 %conv, i64 1, double 8.00e-01)
+  %tobool1 = icmp ne i64 %expval, 0
+; CHECK: !prof !0, !misexpect !1
+; CHECK-NOT: @llvm.expect.with.probability
+  br i1 %tobool1, label %if.then, label %if.end
+
+if.then:  ; preds = %entry
+  %call = call i32 (...) @f()
+  store i32 %call, i32* %retval
+  br label %return
+
+if.end:   ; preds = %entry
+  store i32 1, i32* %retval
+  br label %return
+
+return:   ; preds = %if.end, %if.then
+  %0 = load i32, i32* %retval
+  ret i32 %0
+}
+
+; CHECK-LABEL: @test4(
+define i32 @test4(i32 %x) nounwind uwtable ssp {
+entry:
+  %retval = alloca i32, align 4
+  %x.addr = alloca i32, align 4
+  store i32 %x, i32* %x.addr, align 4
+  %tmp = load i32, i32* %x.addr, align 4
+  %tobool = icmp ne i32 %tmp, 0
+  %lnot = xor i1 %tobool, true
+  %lnot1 = xor i1 %lnot, true
+  %lnot.ext = zext i1 %lnot1 to i32
+  %conv = sext i32 %lnot.ext to i64
+  %expval = call i64 @llvm.expect.with.probability.i64(i64 %conv, i64 1, double 8.00e-01)
+  %tobool2 = icmp ne i64 %expval, 0
+; CHECK: !prof !0, !misexpect !1
+; CHECK-NOT: @llvm.expect.with.probability
+  br i1 %tobool2, label %if.then, label %if.end
+
+if.then:  

[PATCH] D79830: Add support of __builtin_expect_with_probability

[Zhi Zhuang via Phabricator via cfe-commits]

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
  clang/include/clang/Basic/DiagnosticSemaKinds.td
  clang/lib/CodeGen/CGBuiltin.cpp
  clang/lib/Sema/SemaChecking.cpp
  clang/test/CodeGen/builtin-expect-with-probability.cpp
  clang/test/Sema/builtin-expect-with-probability-avr.cpp
  clang/test/Sema/builtin-expect-with-probability.cpp
  llvm/docs/BranchWeightMetadata.rst
  llvm/docs/LangRef.rst
  llvm/include/llvm/IR/Intrinsics.td
  llvm/lib/Transforms/Scalar/LowerExpectIntrinsic.cpp
  llvm/test/Transforms/LowerExpectIntrinsic/expect-with-probability.ll

Index: llvm/test/Transforms/LowerExpectIntrinsic/expect-with-probability.ll
===
--- /dev/null
+++ llvm/test/Transforms/LowerExpectIntrinsic/expect-with-probability.ll
@@ -0,0 +1,295 @@
+; RUN: opt -lower-expect -strip-dead-prototypes -S -o - < %s | FileCheck %s
+; RUN: opt -S -passes='function(lower-expect),strip-dead-prototypes' < %s | FileCheck %s
+
+; CHECK-LABEL: @test1(
+define i32 @test1(i32 %x) nounwind uwtable ssp {
+entry:
+  %retval = alloca i32, align 4
+  %x.addr = alloca i32, align 4
+  store i32 %x, i32* %x.addr, align 4
+  %tmp = load i32, i32* %x.addr, align 4
+  %cmp = icmp sgt i32 %tmp, 1
+  %conv = zext i1 %cmp to i32
+  %conv1 = sext i32 %conv to i64
+  %expval = call i64 @llvm.expect.with.probability.i64(i64 %conv1, i64 1, double 8.00e-01)
+  %tobool = icmp ne i64 %expval, 0
+; CHECK: !prof !0, !misexpect !1
+; CHECK-NOT: @llvm.expect.with.probability
+  br i1 %tobool, label %if.then, label %if.end
+
+if.then:  ; preds = %entry
+  %call = call i32 (...) @f()
+  store i32 %call, i32* %retval
+  br label %return
+
+if.end:   ; preds = %entry
+  store i32 1, i32* %retval
+  br label %return
+
+return:   ; preds = %if.end, %if.then
+  %0 = load i32, i32* %retval
+  ret i32 %0
+}
+
+declare i64 @llvm.expect.with.probability.i64(i64, i64, double) nounwind readnone
+
+declare i32 @f(...)
+
+; CHECK-LABEL: @test2(
+define i32 @test2(i32 %x) nounwind uwtable ssp {
+entry:
+  %retval = alloca i32, align 4
+  %x.addr = alloca i32, align 4
+  store i32 %x, i32* %x.addr, align 4
+  %tmp = load i32, i32* %x.addr, align 4
+  %conv = sext i32 %tmp to i64
+  %expval = call i64 @llvm.expect.with.probability.i64(i64 %conv, i64 1, double 8.00e-01)
+  %tobool = icmp ne i64 %expval, 0
+; CHECK: !prof !0, !misexpect !1
+; CHECK-NOT: @llvm.expect.with.probability
+  br i1 %tobool, label %if.then, label %if.end
+
+if.then:  ; preds = %entry
+  %call = call i32 (...) @f()
+  store i32 %call, i32* %retval
+  br label %return
+
+if.end:   ; preds = %entry
+  store i32 1, i32* %retval
+  br label %return
+
+return:   ; preds = %if.end, %if.then
+  %0 = load i32, i32* %retval
+  ret i32 %0
+}
+
+; CHECK-LABEL: @test3(
+define i32 @test3(i32 %x) nounwind uwtable ssp {
+entry:
+  %retval = alloca i32, align 4
+  %x.addr = alloca i32, align 4
+  store i32 %x, i32* %x.addr, align 4
+  %tmp = load i32, i32* %x.addr, align 4
+  %tobool = icmp ne i32 %tmp, 0
+  %lnot = xor i1 %tobool, true
+  %lnot.ext = zext i1 %lnot to i32
+  %conv = sext i32 %lnot.ext to i64
+  %expval = call i64 @llvm.expect.with.probability.i64(i64 %conv, i64 1, double 8.00e-01)
+  %tobool1 = icmp ne i64 %expval, 0
+; CHECK: !prof !0, !misexpect !1
+; CHECK-NOT: @llvm.expect.with.probability
+  br i1 %tobool1, label %if.then, label %if.end
+
+if.then:  ; preds = %entry
+  %call = call i32 (...) @f()
+  store i32 %call, i32* %retval
+  br label %return
+
+if.end:   ; preds = %entry
+  store i32 1, i32* %retval
+  br label %return
+
+return:   ; preds = %if.end, %if.then
+  %0 = load i32, i32* %retval
+  ret i32 %0
+}
+
+; CHECK-LABEL: @test4(
+define i32 @test4(i32 %x) nounwind uwtable ssp {
+entry:
+  %retval = alloca i32, align 4
+  %x.addr = alloca i32, align 4
+  store i32 %x, i32* %x.addr, align 4
+  %tmp = load i32, i32* %x.addr, align 4
+  %tobool = icmp ne i32 %tmp, 0
+  %lnot = xor i1 %tobool, true
+  %lnot1 = xor i1 %lnot, true
+  %lnot.ext = zext i1 %lnot1 to i32
+  %conv = sext i32 %lnot.ext to i64
+  %expval = call i64 @llvm.expect.with.probability.i64(i64 %conv, i64 1, double 8.00e-01)
+  %tobool2 = icmp ne i64 %expval, 0
+; CHECK: !prof !0, !misexpect !1
+; CHECK-NOT: @llvm.expect.with.probability
+  br i1 %tobool2, label %if.then, label %if.end
+
+if.then:  ; preds = %entry
+ 

[PATCH] D79830: Add support of __builtin_expect_with_probability

[Zhi Zhuang via Phabricator via cfe-commits]

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
  clang/include/clang/Basic/DiagnosticSemaKinds.td
  clang/lib/CodeGen/CGBuiltin.cpp
  clang/lib/Sema/SemaChecking.cpp
  clang/test/CodeGen/builtin-expect-with-probability.cpp
  clang/test/Sema/builtin-expect-with-probability-avr.cpp
  clang/test/Sema/builtin-expect-with-probability.cpp
  llvm/docs/BranchWeightMetadata.rst
  llvm/docs/LangRef.rst
  llvm/include/llvm/IR/Intrinsics.td
  llvm/lib/Transforms/Scalar/LowerExpectIntrinsic.cpp
  llvm/test/Transforms/LowerExpectIntrinsic/expect-with-probability.ll

Index: llvm/test/Transforms/LowerExpectIntrinsic/expect-with-probability.ll
===
--- /dev/null
+++ llvm/test/Transforms/LowerExpectIntrinsic/expect-with-probability.ll
@@ -0,0 +1,295 @@
+; RUN: opt -lower-expect -strip-dead-prototypes -S -o - < %s | FileCheck %s
+; RUN: opt -S -passes='function(lower-expect),strip-dead-prototypes' < %s | FileCheck %s
+
+; CHECK-LABEL: @test1(
+define i32 @test1(i32 %x) nounwind uwtable ssp {
+entry:
+  %retval = alloca i32, align 4
+  %x.addr = alloca i32, align 4
+  store i32 %x, i32* %x.addr, align 4
+  %tmp = load i32, i32* %x.addr, align 4
+  %cmp = icmp sgt i32 %tmp, 1
+  %conv = zext i1 %cmp to i32
+  %conv1 = sext i32 %conv to i64
+  %expval = call i64 @llvm.expect.with.probability.i64(i64 %conv1, i64 1, double 8.00e-01)
+  %tobool = icmp ne i64 %expval, 0
+; CHECK: !prof !0, !misexpect !1
+; CHECK-NOT: @llvm.expect.with.probability
+  br i1 %tobool, label %if.then, label %if.end
+
+if.then:  ; preds = %entry
+  %call = call i32 (...) @f()
+  store i32 %call, i32* %retval
+  br label %return
+
+if.end:   ; preds = %entry
+  store i32 1, i32* %retval
+  br label %return
+
+return:   ; preds = %if.end, %if.then
+  %0 = load i32, i32* %retval
+  ret i32 %0
+}
+
+declare i64 @llvm.expect.with.probability.i64(i64, i64, double) nounwind readnone
+
+declare i32 @f(...)
+
+; CHECK-LABEL: @test2(
+define i32 @test2(i32 %x) nounwind uwtable ssp {
+entry:
+  %retval = alloca i32, align 4
+  %x.addr = alloca i32, align 4
+  store i32 %x, i32* %x.addr, align 4
+  %tmp = load i32, i32* %x.addr, align 4
+  %conv = sext i32 %tmp to i64
+  %expval = call i64 @llvm.expect.with.probability.i64(i64 %conv, i64 1, double 8.00e-01)
+  %tobool = icmp ne i64 %expval, 0
+; CHECK: !prof !0, !misexpect !1
+; CHECK-NOT: @llvm.expect.with.probability
+  br i1 %tobool, label %if.then, label %if.end
+
+if.then:  ; preds = %entry
+  %call = call i32 (...) @f()
+  store i32 %call, i32* %retval
+  br label %return
+
+if.end:   ; preds = %entry
+  store i32 1, i32* %retval
+  br label %return
+
+return:   ; preds = %if.end, %if.then
+  %0 = load i32, i32* %retval
+  ret i32 %0
+}
+
+; CHECK-LABEL: @test3(
+define i32 @test3(i32 %x) nounwind uwtable ssp {
+entry:
+  %retval = alloca i32, align 4
+  %x.addr = alloca i32, align 4
+  store i32 %x, i32* %x.addr, align 4
+  %tmp = load i32, i32* %x.addr, align 4
+  %tobool = icmp ne i32 %tmp, 0
+  %lnot = xor i1 %tobool, true
+  %lnot.ext = zext i1 %lnot to i32
+  %conv = sext i32 %lnot.ext to i64
+  %expval = call i64 @llvm.expect.with.probability.i64(i64 %conv, i64 1, double 8.00e-01)
+  %tobool1 = icmp ne i64 %expval, 0
+; CHECK: !prof !0, !misexpect !1
+; CHECK-NOT: @llvm.expect.with.probability
+  br i1 %tobool1, label %if.then, label %if.end
+
+if.then:  ; preds = %entry
+  %call = call i32 (...) @f()
+  store i32 %call, i32* %retval
+  br label %return
+
+if.end:   ; preds = %entry
+  store i32 1, i32* %retval
+  br label %return
+
+return:   ; preds = %if.end, %if.then
+  %0 = load i32, i32* %retval
+  ret i32 %0
+}
+
+; CHECK-LABEL: @test4(
+define i32 @test4(i32 %x) nounwind uwtable ssp {
+entry:
+  %retval = alloca i32, align 4
+  %x.addr = alloca i32, align 4
+  store i32 %x, i32* %x.addr, align 4
+  %tmp = load i32, i32* %x.addr, align 4
+  %tobool = icmp ne i32 %tmp, 0
+  %lnot = xor i1 %tobool, true
+  %lnot1 = xor i1 %lnot, true
+  %lnot.ext = zext i1 %lnot1 to i32
+  %conv = sext i32 %lnot.ext to i64
+  %expval = call i64 @llvm.expect.with.probability.i64(i64 %conv, i64 1, double 8.00e-01)
+  %tobool2 = icmp ne i64 %expval, 0
+; CHECK: !prof !0, !misexpect !1
+; CHECK-NOT: @llvm.expect.with.probability
+  br i1 %tobool2, label %if.then, label %if.end
+
+if.then:  ; preds = %entry
+  

[PATCH] D79830: Add support of __builtin_expect_with_probability

[Zhi Zhuang via Phabricator via cfe-commits]

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
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D83362: Fix warning caused by __builtin_expect_with_probability was not handled in places such as constant folding

[Zhi Zhuang via Phabricator via cfe-commits]

LukeZhuang created this revision.
LukeZhuang added reviewers: lebedev.ri, erichkeane, xbolva00, RKSimon, rsmith.
LukeZhuang added a project: clang.
Herald added subscribers: cfe-commits, martong.

Previously some places that should have handled 
`__builtin_expect_with_probability` is missing, so in some case it acts 
differently than `__builtin_expect`.
For example it was not handled in constant folding, thus in the following 
program, the "if" condition should be constantly true and folded, but 
previously it was not handled and cause warning "control may reach end of 
non-void function" (while `__builtin_expect` does not):

  __attribute__((noreturn)) extern void bar();
  int foo(int x, int y) {
if (y) {
  if (__builtin_expect_with_probability(1, 1, 1))
bar();
}
 else
  return 0;
  }

Now it's fixed.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D83362

Files:
  clang/lib/AST/ExprConstant.cpp
  clang/lib/StaticAnalyzer/Checkers/BuiltinFunctionChecker.cpp
  clang/test/Sema/builtin-expect-with-probability.cpp


Index: clang/test/Sema/builtin-expect-with-probability.cpp
===
--- clang/test/Sema/builtin-expect-with-probability.cpp
+++ clang/test/Sema/builtin-expect-with-probability.cpp
@@ -1,4 +1,16 @@
 // RUN: %clang_cc1 -fsyntax-only -verify %s
+
+__attribute__((noreturn)) extern void bar();
+
+int test_no_warn(int x) {
+  if (x) {
+if (__builtin_expect_with_probability(1, 1, 1))
+  bar();
+  } else {
+return 0;
+  }
+}  // should not emit warn "control may reach end of non-void function" here 
since expr is constantly true, so the "if(__bui..)" should be constantly true 
condition and be ignored
+
 extern int global;
 
 struct S {
Index: clang/lib/StaticAnalyzer/Checkers/BuiltinFunctionChecker.cpp
===
--- clang/lib/StaticAnalyzer/Checkers/BuiltinFunctionChecker.cpp
+++ clang/lib/StaticAnalyzer/Checkers/BuiltinFunctionChecker.cpp
@@ -64,10 +64,12 @@
 
   case Builtin::BI__builtin_unpredictable:
   case Builtin::BI__builtin_expect:
+  case Builtin::BI__builtin_expect_with_probability:
   case Builtin::BI__builtin_assume_aligned:
   case Builtin::BI__builtin_addressof: {
-// For __builtin_unpredictable, __builtin_expect, and
-// __builtin_assume_aligned, just return the value of the subexpression.
+// For __builtin_unpredictable, __builtin_expect,
+// __builtin_expect_with_probability and __builtin_assume_aligned,
+// just return the value of the subexpression.
 // __builtin_addressof is going from a reference to a pointer, but those
 // are represented the same way in the analyzer.
 assert (Call.getNumArgs() > 0);
Index: clang/lib/AST/ExprConstant.cpp
===
--- clang/lib/AST/ExprConstant.cpp
+++ clang/lib/AST/ExprConstant.cpp
@@ -11200,6 +11200,7 @@
   }
 
   case Builtin::BI__builtin_expect:
+  case Builtin::BI__builtin_expect_with_probability:
 return Visit(E->getArg(0));
 
   case Builtin::BI__builtin_ffs:


Index: clang/test/Sema/builtin-expect-with-probability.cpp
===
--- clang/test/Sema/builtin-expect-with-probability.cpp
+++ clang/test/Sema/builtin-expect-with-probability.cpp
@@ -1,4 +1,16 @@
 // RUN: %clang_cc1 -fsyntax-only -verify %s
+
+__attribute__((noreturn)) extern void bar();
+
+int test_no_warn(int x) {
+  if (x) {
+if (__builtin_expect_with_probability(1, 1, 1))
+  bar();
+  } else {
+return 0;
+  }
+}  // should not emit warn "control may reach end of non-void function" here since expr is constantly true, so the "if(__bui..)" should be constantly true condition and be ignored
+
 extern int global;
 
 struct S {
Index: clang/lib/StaticAnalyzer/Checkers/BuiltinFunctionChecker.cpp
===
--- clang/lib/StaticAnalyzer/Checkers/BuiltinFunctionChecker.cpp
+++ clang/lib/StaticAnalyzer/Checkers/BuiltinFunctionChecker.cpp
@@ -64,10 +64,12 @@
 
   case Builtin::BI__builtin_unpredictable:
   case Builtin::BI__builtin_expect:
+  case Builtin::BI__builtin_expect_with_probability:
   case Builtin::BI__builtin_assume_aligned:
   case Builtin::BI__builtin_addressof: {
-// For __builtin_unpredictable, __builtin_expect, and
-// __builtin_assume_aligned, just return the value of the subexpression.
+// For __builtin_unpredictable, __builtin_expect,
+// __builtin_expect_with_probability and __builtin_assume_aligned,
+// just return the value of the subexpression.
 // __builtin_addressof is going from a reference to a pointer, but those
 // are represented the same way in the analyzer.
 assert (Call.getNumArgs() > 0);
Index: clang/lib/AST/ExprConstant.cpp
===
--- clang/lib/AST/ExprConstant.cpp
+++ clang/

[PATCH] D83362: Fix warning caused by __builtin_expect_with_probability was not handled in places such as constant folding

[Zhi Zhuang via Phabricator via cfe-commits]

LukeZhuang updated this revision to Diff 276517.
LukeZhuang added a comment.

**updated: 07/08/2020**
(1) improve test case


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

https://reviews.llvm.org/D83362

Files:
  clang/lib/AST/ExprConstant.cpp
  clang/lib/StaticAnalyzer/Checkers/BuiltinFunctionChecker.cpp
  clang/test/Sema/builtin-expect-with-probability.cpp


Index: clang/test/Sema/builtin-expect-with-probability.cpp
===
--- clang/test/Sema/builtin-expect-with-probability.cpp
+++ clang/test/Sema/builtin-expect-with-probability.cpp
@@ -1,4 +1,26 @@
 // RUN: %clang_cc1 -fsyntax-only -verify %s
+
+__attribute__((noreturn)) extern void bar();
+
+int test_no_warn(int x) {
+  if (x) {
+if (__builtin_expect_with_probability(1, 1, 1))
+  bar();
+  } else {
+return 0;
+  }
+} // should not emit warn "control may reach end of non-void function" here 
since expr is constantly true, so the "if(__bui..)" should be constantly true 
condition and be ignored
+
+extern void f(int x);
+
+constexpr int constf() {
+  return __builtin_expect_with_probability(1, 1, 1); // should not have error 
here
+}
+
+void foo() {
+  f(constf());
+}
+
 extern int global;
 
 struct S {
Index: clang/lib/StaticAnalyzer/Checkers/BuiltinFunctionChecker.cpp
===
--- clang/lib/StaticAnalyzer/Checkers/BuiltinFunctionChecker.cpp
+++ clang/lib/StaticAnalyzer/Checkers/BuiltinFunctionChecker.cpp
@@ -64,10 +64,12 @@
 
   case Builtin::BI__builtin_unpredictable:
   case Builtin::BI__builtin_expect:
+  case Builtin::BI__builtin_expect_with_probability:
   case Builtin::BI__builtin_assume_aligned:
   case Builtin::BI__builtin_addressof: {
-// For __builtin_unpredictable, __builtin_expect, and
-// __builtin_assume_aligned, just return the value of the subexpression.
+// For __builtin_unpredictable, __builtin_expect,
+// __builtin_expect_with_probability and __builtin_assume_aligned,
+// just return the value of the subexpression.
 // __builtin_addressof is going from a reference to a pointer, but those
 // are represented the same way in the analyzer.
 assert (Call.getNumArgs() > 0);
Index: clang/lib/AST/ExprConstant.cpp
===
--- clang/lib/AST/ExprConstant.cpp
+++ clang/lib/AST/ExprConstant.cpp
@@ -11200,6 +11200,7 @@
   }
 
   case Builtin::BI__builtin_expect:
+  case Builtin::BI__builtin_expect_with_probability:
 return Visit(E->getArg(0));
 
   case Builtin::BI__builtin_ffs:


Index: clang/test/Sema/builtin-expect-with-probability.cpp
===
--- clang/test/Sema/builtin-expect-with-probability.cpp
+++ clang/test/Sema/builtin-expect-with-probability.cpp
@@ -1,4 +1,26 @@
 // RUN: %clang_cc1 -fsyntax-only -verify %s
+
+__attribute__((noreturn)) extern void bar();
+
+int test_no_warn(int x) {
+  if (x) {
+if (__builtin_expect_with_probability(1, 1, 1))
+  bar();
+  } else {
+return 0;
+  }
+} // should not emit warn "control may reach end of non-void function" here since expr is constantly true, so the "if(__bui..)" should be constantly true condition and be ignored
+
+extern void f(int x);
+
+constexpr int constf() {
+  return __builtin_expect_with_probability(1, 1, 1); // should not have error here
+}
+
+void foo() {
+  f(constf());
+}
+
 extern int global;
 
 struct S {
Index: clang/lib/StaticAnalyzer/Checkers/BuiltinFunctionChecker.cpp
===
--- clang/lib/StaticAnalyzer/Checkers/BuiltinFunctionChecker.cpp
+++ clang/lib/StaticAnalyzer/Checkers/BuiltinFunctionChecker.cpp
@@ -64,10 +64,12 @@
 
   case Builtin::BI__builtin_unpredictable:
   case Builtin::BI__builtin_expect:
+  case Builtin::BI__builtin_expect_with_probability:
   case Builtin::BI__builtin_assume_aligned:
   case Builtin::BI__builtin_addressof: {
-// For __builtin_unpredictable, __builtin_expect, and
-// __builtin_assume_aligned, just return the value of the subexpression.
+// For __builtin_unpredictable, __builtin_expect,
+// __builtin_expect_with_probability and __builtin_assume_aligned,
+// just return the value of the subexpression.
 // __builtin_addressof is going from a reference to a pointer, but those
 // are represented the same way in the analyzer.
 assert (Call.getNumArgs() > 0);
Index: clang/lib/AST/ExprConstant.cpp
===
--- clang/lib/AST/ExprConstant.cpp
+++ clang/lib/AST/ExprConstant.cpp
@@ -11200,6 +11200,7 @@
   }
 
   case Builtin::BI__builtin_expect:
+  case Builtin::BI__builtin_expect_with_probability:
 return Visit(E->getArg(0));
 
   case Builtin::BI__builtin_ffs:
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi

[PATCH] D83362: Fix warning caused by __builtin_expect_with_probability was not handled in places such as constant folding

[Zhi Zhuang via Phabricator via cfe-commits]

LukeZhuang marked an inline comment as done.
LukeZhuang added a comment.

@erichkeane Thank you very much for the comments. I have added test case to 
make sure it is evaluated during compile time. Previous code could not pass 
these test cases.


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

https://reviews.llvm.org/D83362



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D83362: Fix warning caused by __builtin_expect_with_probability was not handled in places such as constant folding

[Zhi Zhuang via Phabricator via cfe-commits]

LukeZhuang updated this revision to Diff 276557.
LukeZhuang added a comment.

**updated: 07/08/2020**
(1) improve test case


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

https://reviews.llvm.org/D83362

Files:
  clang/lib/AST/ExprConstant.cpp
  clang/lib/StaticAnalyzer/Checkers/BuiltinFunctionChecker.cpp
  clang/test/Sema/builtin-expect-with-probability.cpp


Index: clang/test/Sema/builtin-expect-with-probability.cpp
===
--- clang/test/Sema/builtin-expect-with-probability.cpp
+++ clang/test/Sema/builtin-expect-with-probability.cpp
@@ -1,4 +1,30 @@
 // RUN: %clang_cc1 -fsyntax-only -verify %s
+
+__attribute__((noreturn)) extern void bar();
+
+int test_no_warn(int x) {
+  if (x) {
+if (__builtin_expect_with_probability(1, 1, 1))
+  bar();
+  } else {
+return 0;
+  }
+} // should not emit warn "control may reach end of non-void function" here 
since expr is constantly true, so the "if(__bui..)" should be constantly true 
condition and be ignored
+
+template  void tempf() {
+  static_assert(b == 1, "should be evaluated as 1"); // should not have error 
here
+}
+
+constexpr int constf() {
+  return __builtin_expect_with_probability(1, 1, 1);
+}
+
+void foo() {
+  tempf<__builtin_expect_with_probability(1, 1, 1)>();
+  constexpr int f = constf();
+  static_assert(f == 1, "should be evaluated as 1"); // should not have error 
here
+}
+
 extern int global;
 
 struct S {
Index: clang/lib/StaticAnalyzer/Checkers/BuiltinFunctionChecker.cpp
===
--- clang/lib/StaticAnalyzer/Checkers/BuiltinFunctionChecker.cpp
+++ clang/lib/StaticAnalyzer/Checkers/BuiltinFunctionChecker.cpp
@@ -64,10 +64,12 @@
 
   case Builtin::BI__builtin_unpredictable:
   case Builtin::BI__builtin_expect:
+  case Builtin::BI__builtin_expect_with_probability:
   case Builtin::BI__builtin_assume_aligned:
   case Builtin::BI__builtin_addressof: {
-// For __builtin_unpredictable, __builtin_expect, and
-// __builtin_assume_aligned, just return the value of the subexpression.
+// For __builtin_unpredictable, __builtin_expect,
+// __builtin_expect_with_probability and __builtin_assume_aligned,
+// just return the value of the subexpression.
 // __builtin_addressof is going from a reference to a pointer, but those
 // are represented the same way in the analyzer.
 assert (Call.getNumArgs() > 0);
Index: clang/lib/AST/ExprConstant.cpp
===
--- clang/lib/AST/ExprConstant.cpp
+++ clang/lib/AST/ExprConstant.cpp
@@ -11200,6 +11200,7 @@
   }
 
   case Builtin::BI__builtin_expect:
+  case Builtin::BI__builtin_expect_with_probability:
 return Visit(E->getArg(0));
 
   case Builtin::BI__builtin_ffs:


Index: clang/test/Sema/builtin-expect-with-probability.cpp
===
--- clang/test/Sema/builtin-expect-with-probability.cpp
+++ clang/test/Sema/builtin-expect-with-probability.cpp
@@ -1,4 +1,30 @@
 // RUN: %clang_cc1 -fsyntax-only -verify %s
+
+__attribute__((noreturn)) extern void bar();
+
+int test_no_warn(int x) {
+  if (x) {
+if (__builtin_expect_with_probability(1, 1, 1))
+  bar();
+  } else {
+return 0;
+  }
+} // should not emit warn "control may reach end of non-void function" here since expr is constantly true, so the "if(__bui..)" should be constantly true condition and be ignored
+
+template  void tempf() {
+  static_assert(b == 1, "should be evaluated as 1"); // should not have error here
+}
+
+constexpr int constf() {
+  return __builtin_expect_with_probability(1, 1, 1);
+}
+
+void foo() {
+  tempf<__builtin_expect_with_probability(1, 1, 1)>();
+  constexpr int f = constf();
+  static_assert(f == 1, "should be evaluated as 1"); // should not have error here
+}
+
 extern int global;
 
 struct S {
Index: clang/lib/StaticAnalyzer/Checkers/BuiltinFunctionChecker.cpp
===
--- clang/lib/StaticAnalyzer/Checkers/BuiltinFunctionChecker.cpp
+++ clang/lib/StaticAnalyzer/Checkers/BuiltinFunctionChecker.cpp
@@ -64,10 +64,12 @@
 
   case Builtin::BI__builtin_unpredictable:
   case Builtin::BI__builtin_expect:
+  case Builtin::BI__builtin_expect_with_probability:
   case Builtin::BI__builtin_assume_aligned:
   case Builtin::BI__builtin_addressof: {
-// For __builtin_unpredictable, __builtin_expect, and
-// __builtin_assume_aligned, just return the value of the subexpression.
+// For __builtin_unpredictable, __builtin_expect,
+// __builtin_expect_with_probability and __builtin_assume_aligned,
+// just return the value of the subexpression.
 // __builtin_addressof is going from a reference to a pointer, but those
 // are represented the same way in the analyzer.
 assert (Call.getNumArgs() > 0);
Index: clang/lib/AST/ExprConstant.cpp

[PATCH] D83362: Fix warning caused by __builtin_expect_with_probability was not handled in places such as constant folding

[Zhi Zhuang via Phabricator via cfe-commits]

LukeZhuang marked an inline comment as done.
LukeZhuang added a comment.

Thank you for the new comment. I've modified my test cases


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

https://reviews.llvm.org/D83362



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D83362: Fix warning caused by __builtin_expect_with_probability was not handled in places such as constant folding

[Zhi Zhuang via Phabricator via cfe-commits]

LukeZhuang added a comment.

Hi, could you please help me commit this change? Thank you very much!


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

https://reviews.llvm.org/D83362



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D83362: Fix warning caused by __builtin_expect_with_probability was not handled in places such as constant folding

[Zhi Zhuang via Phabricator via cfe-commits]

LukeZhuang added a comment.

Thank you very much!


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

https://reviews.llvm.org/D83362



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D79830: Add support of __builtin_expect_with_probability

[Zhi Zhuang via Phabricator via cfe-commits]

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



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D79830: Add support of __builtin_expect_with_probability

[Zhi Zhuang via Phabricator via cfe-commits]

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
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D79830: Add support of __builtin_expect_with_probability

[Zhi Zhuang via Phabricator via cfe-commits]

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 :
>  
> 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



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D82403: fix test failure for clang/test/CodeGen/builtin-expect-with-probability.cpp

[Zhi Zhuang via Phabricator via cfe-commits]

LukeZhuang created this revision.
LukeZhuang added a reviewer: erichkeane.
LukeZhuang added projects: LLVM, clang.
Herald added a subscriber: cfe-commits.
LukeZhuang edited the summary of this revision.

Fix test case added by D79830 
Rewrite the test case, which did similar thing as builtin-expect.c does(test 
generated llvm intrinsic instead of test branch weights). 
Currently pass by "-disable-llvm-passes" option.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D82403

Files:
  clang/test/CodeGen/builtin-expect-with-probability.cpp

Index: clang/test/CodeGen/builtin-expect-with-probability.cpp
===
--- clang/test/CodeGen/builtin-expect-with-probability.cpp
+++ clang/test/CodeGen/builtin-expect-with-probability.cpp
@@ -1,14 +1,38 @@
-// RUN: %clang_cc1 -emit-llvm -o - %s -O1 | FileCheck %s
-// RUN: %clang_cc1 -emit-llvm -o - -fexperimental-new-pass-manager %s -O1 | FileCheck %s
+// RUN: %clang_cc1 -triple x86_64-unknown-unknown -emit-llvm -o - %s -O1 -disable-llvm-passes | FileCheck %s --check-prefix=ALL --check-prefix=O1
+// RUN: %clang_cc1 -triple x86_64-unknown-unknown -emit-llvm -o - %s -O0 | FileCheck %s --check-prefix=ALL --check-prefix=O0
 extern int global;
 
+int expect_taken(int x) {
+// ALL-LABEL: expect_taken
+// O1:call i64 @llvm.expect.with.probability.i64(i64 {{%.*}}, i64 1, double 9.00e-01)
+// O0-NOT:@llvm.expect.with.probability
+
+  if (__builtin_expect_with_probability(x == 100, 1, 0.9)) {
+return 0;
+  }
+  return x;
+}
+
+int expect_not_taken(int x) {
+// ALL-LABEL: expect_not_taken
+// O1:call i64 @llvm.expect.with.probability.i64(i64 {{%.*}}, i64 0, double 9.00e-01)
+// O0-NOT:@llvm.expect.with.probability
+
+  if (__builtin_expect_with_probability(x == 100, 0, 0.9)) {
+return 0;
+  }
+  return x;
+}
+
 struct S {
   static constexpr int prob = 1;
 };
 
 template
-int expect_taken(int x) {
-// CHECK: !{{[0-9]+}} = !{!"branch_weights", i32 2147483647, i32 1}
+int expect_taken_template(int x) {
+// ALL-LABEL: expect_taken_template
+// O1:call i64 @llvm.expect.with.probability.i64(i64 {{%.*}}, i64 1, double 1.00e+00)
+// O0-NOT:@llvm.expect.with.probability
 
 	if (__builtin_expect_with_probability (x == 100, 1, T::prob)) {
 		return 0;
@@ -17,20 +41,29 @@
 }
 
 int f() {
-  return expect_taken(global);
+  return expect_taken_template(global);
 }
 
-int expect_taken2(int x) {
-  // CHECK: !{{[0-9]+}} = !{!"branch_weights", i32 1932735283, i32 214748366}
+int x;
+int y(void);
+void foo();
 
-  if (__builtin_expect_with_probability(x == 100, 1, 0.9)) {
-return 0;
-  }
-  return x;
+void expect_value_side_effects() {
+// ALL-LABEL: expect_value_side_effects
+// ALL: [[CALL:%.*]] = call i32 @{{.*}}y
+// O1:  [[SEXT:%.*]] = sext i32 [[CALL]] to i64
+// O1:  call i64 @llvm.expect.with.probability.i64(i64 {{%.*}}, i64 [[SEXT]], double 6.00e-01)
+// O0-NOT: @llvm.expect.with.probability
+
+  if (__builtin_expect_with_probability(x, y(), 0.6))
+foo();
 }
 
-int expect_taken3(int x) {
-  // CHECK: !{{[0-9]+}} = !{!"branch_weights", i32 107374184, i32 107374184, i32 1717986918, i32 107374184, i32 107374184}
+int switch_cond(int x) {
+// ALL-LABEL: switch_cond
+// O1:call i64 @llvm.expect.with.probability.i64(i64 {{%.*}}, i64 1, double 8.00e-01)
+// O0-NOT:@llvm.expect.with.probability
+
   switch (__builtin_expect_with_probability(x, 1, 0.8)) {
   case 0:
 x = x + 0;
@@ -45,3 +78,22 @@
   }
   return x;
 }
+
+constexpr double prob = 0.8;
+
+int variable_expected(int stuff) {
+// ALL-LABEL: variable_expected
+// O1: call i64 @llvm.expect.with.probability.i64(i64 {{%.*}}, i64 {{%.*}}, double 8.00e-01)
+// O0-NOT: @llvm.expect.with.probability
+
+  int res = 0;
+
+  switch(__builtin_expect_with_probability(stuff, stuff, prob)) {
+case 0:
+  res = 1;
+  break;
+default:
+  break;
+  }
+  return res;
+}
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D82403: fix test failure for clang/test/CodeGen/builtin-expect-with-probability.cpp

[Zhi Zhuang via Phabricator via cfe-commits]

LukeZhuang updated this revision to Diff 272805.
LukeZhuang added a comment.

Fixed. If it looks good to you, could you please help me commit it? Thank you 
very much!


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

https://reviews.llvm.org/D82403

Files:
  clang/test/CodeGen/builtin-expect-with-probability.cpp

Index: clang/test/CodeGen/builtin-expect-with-probability.cpp
===
--- clang/test/CodeGen/builtin-expect-with-probability.cpp
+++ clang/test/CodeGen/builtin-expect-with-probability.cpp
@@ -1,14 +1,38 @@
-// RUN: %clang_cc1 -emit-llvm -o - %s -O1 | FileCheck %s
-// RUN: %clang_cc1 -emit-llvm -o - -fexperimental-new-pass-manager %s -O1 | FileCheck %s
+// RUN: %clang_cc1 -triple x86_64-unknown-unknown -emit-llvm -o - %s -O1 -disable-llvm-passes | FileCheck %s --check-prefix=ALL --check-prefix=O1
+// RUN: %clang_cc1 -triple x86_64-unknown-unknown -emit-llvm -o - %s -O0 | FileCheck %s --check-prefix=ALL --check-prefix=O0
 extern int global;
 
+int expect_taken(int x) {
+// ALL-LABEL: expect_taken
+// O1:call i64 @llvm.expect.with.probability.i64(i64 {{%.*}}, i64 1, double 9.00e-01)
+// O0-NOT:@llvm.expect.with.probability
+
+  if (__builtin_expect_with_probability(x == 100, 1, 0.9)) {
+return 0;
+  }
+  return x;
+}
+
+int expect_not_taken(int x) {
+// ALL-LABEL: expect_not_taken
+// O1:call i64 @llvm.expect.with.probability.i64(i64 {{%.*}}, i64 0, double 9.00e-01)
+// O0-NOT:@llvm.expect.with.probability
+
+  if (__builtin_expect_with_probability(x == 100, 0, 0.9)) {
+return 0;
+  }
+  return x;
+}
+
 struct S {
   static constexpr int prob = 1;
 };
 
 template
-int expect_taken(int x) {
-// CHECK: !{{[0-9]+}} = !{!"branch_weights", i32 2147483647, i32 1}
+int expect_taken_template(int x) {
+// ALL-LABEL: expect_taken_template
+// O1:call i64 @llvm.expect.with.probability.i64(i64 {{%.*}}, i64 1, double 1.00e+00)
+// O0-NOT:@llvm.expect.with.probability
 
 	if (__builtin_expect_with_probability (x == 100, 1, T::prob)) {
 		return 0;
@@ -17,20 +41,31 @@
 }
 
 int f() {
-  return expect_taken(global);
+  return expect_taken_template(global);
 }
 
-int expect_taken2(int x) {
-  // CHECK: !{{[0-9]+}} = !{!"branch_weights", i32 1932735283, i32 214748366}
+int x;
+extern "C" {
+  int y(void);
+}
+void foo();
 
-  if (__builtin_expect_with_probability(x == 100, 1, 0.9)) {
-return 0;
-  }
-  return x;
+void expect_value_side_effects() {
+// ALL-LABEL: expect_value_side_effects
+// ALL: [[CALL:%.*]] = call i32 @y
+// O1:  [[SEXT:%.*]] = sext i32 [[CALL]] to i64
+// O1:  call i64 @llvm.expect.with.probability.i64(i64 {{%.*}}, i64 [[SEXT]], double 6.00e-01)
+// O0-NOT: @llvm.expect.with.probability
+
+  if (__builtin_expect_with_probability(x, y(), 0.6))
+foo();
 }
 
-int expect_taken3(int x) {
-  // CHECK: !{{[0-9]+}} = !{!"branch_weights", i32 107374184, i32 107374184, i32 1717986918, i32 107374184, i32 107374184}
+int switch_cond(int x) {
+// ALL-LABEL: switch_cond
+// O1:call i64 @llvm.expect.with.probability.i64(i64 {{%.*}}, i64 1, double 8.00e-01)
+// O0-NOT:@llvm.expect.with.probability
+
   switch (__builtin_expect_with_probability(x, 1, 0.8)) {
   case 0:
 x = x + 0;
@@ -45,3 +80,22 @@
   }
   return x;
 }
+
+constexpr double prob = 0.8;
+
+int variable_expected(int stuff) {
+// ALL-LABEL: variable_expected
+// O1: call i64 @llvm.expect.with.probability.i64(i64 {{%.*}}, i64 {{%.*}}, double 8.00e-01)
+// O0-NOT: @llvm.expect.with.probability
+
+  int res = 0;
+
+  switch(__builtin_expect_with_probability(stuff, stuff, prob)) {
+case 0:
+  res = 1;
+  break;
+default:
+  break;
+  }
+  return res;
+}
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D79830: Add support of __builtin_expect_with_probability

[Zhi Zhuang via Phabricator via cfe-commits]

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 `__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 .


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D79830

Files:
  clang/include/clang/Basic/Builtins.def
  clang/lib/CodeGen/CGBuiltin.cpp
  clang/test/CodeGen/builtin-expect-with-probability-switch.c
  clang/test/CodeGen/builtin-expect-with-probability.c
  llvm/include/llvm/IR/Intrinsics.td
  llvm/lib/Transforms/Scalar/LowerExpectIntrinsic.cpp

Index: clang/test/CodeGen/builtin-expect-with-probability.c
===
--- /dev/null
+++ clang/test/CodeGen/builtin-expect-with-probability.c
@@ -0,0 +1,11 @@
+// RUN: %clang_cc1 -emit-llvm -o - %s -O1 | FileCheck %s
+
+int expect_taken(int x) {
+// CHECK: !{{[0-9]+}} = !{!"branch_weights", i32 1932735283, i32 214748366}
+
+	if (__builtin_expect_with_probability (x == 100, 1, 0.9)) {
+		return 0;
+	}
+	return x;
+}
+
Index: clang/test/CodeGen/builtin-expect-with-probability-switch.c
===
--- /dev/null
+++ clang/test/CodeGen/builtin-expect-with-probability-switch.c
@@ -0,0 +1,19 @@
+// RUN: %clang_cc1 -emit-llvm -o - %s -O1 | FileCheck %s
+
+int expect_taken(int x) {
+// CHECK: !{{[0-9]+}} = !{!"branch_weights", i32 107374184, i32 107374184, i32 1717986918, i32 107374184, i32 107374184}
+	switch(__builtin_expect_with_probability(x, 1, 0.8)) {
+		case 0:
+			x = x + 0;
+		case 1:
+			x = x + 1;
+		case 2:
+			x = x + 2;
+		case 5:
+			x = x + 5;
+		default:
+			x = x + 6;
+	}
+	return x;
+}
+
Index: clang/lib/CodeGen/CGBuiltin.cpp
===
--- clang/lib/CodeGen/CGBuiltin.cpp
+++ clang/lib/CodeGen/CGBuiltin.cpp
@@ -2039,6 +2039,37 @@
 Builder.CreateCall(FnExpect, {ArgValue, ExpectedValue}, "expval");
 return RValue::get(Result);
   }
+  case Builtin::BI__builtin_expect_with_probability: {
+Value *ArgValue = EmitScalarExpr(E->getArg(0));
+llvm::Type *ArgType = ArgValue->getType();
+
+Value *ExpectedValue = EmitScalarExpr(E->getArg(1));
+Value *Confidence = EmitScalarExpr(E->getArg(2));
+ConstantFP *Confidence_f = dyn_cast(Confidence);
+if (!Confidence_f) {
+  CGM.Error(E->getArg(2)->getLocStart(),
+"probability of __builtin_expect_with_probability must be "
+"constant floating-point expression");
+} else {
+  double prob = Confidence_f->getValueAPF().convertToDouble();
+  if (prob < 0.0 || prob > 1.0) {
+CGM.Error(E->getArg(2)->getLocStart(),
+  "probability of __builtin_expect_with_probability is "
+  "outside the range [0.0, 1.0]");
+  }
+}
+// Don't generate llvm.expect.with.probability on -O0 as the backend
+// won't use it for anything.
+// Note, we still IRGen ExpectedValue because it could have side-effects.
+if (CGM.getCodeGenOpts().OptimizationLevel == 0)
+  return RValue::get(ArgValue);
+
+Value *FnExpect =
+CGM.getIntrinsic(Intrinsic::expect_with_probability, ArgType);
+Value *Result = Builder.CreateCall(
+FnExpect, {ArgValue, ExpectedValue, Confidence}, "expval");
+return RValue::get(Result);
+  }
   case Builtin::BI__builtin_assume_aligned: {
 const Expr *Ptr = E->getArg(0);
 Value *PtrValue = EmitScalarExpr(Ptr);
Index: clang/include/clang/Basic/Builtins.def
===
--- clang/include/clang/Basic/Builtins.def
+++ clang/include/clang/Basic/Builtins.def
@@ -562,6 +562,7 @@
 
 BUILTIN(__builtin_unpredictable, "LiLi"   , "nc")
 BUILTIN(__builtin_expect, "LiLiLi"   , "nc")
+BUILTIN(__builtin_expect_with_probability, "LiLiLid"   , "nc")
 BUILTIN(__builtin_prefetch, "vvC*.", "nc")
 BUILTIN(__builtin_readcyclecounter, "ULLi", "n")
 BUILTIN(__builtin_trap, "v", "nr")
Index: llvm/lib/Transforms/Scalar/LowerExpectIntrinsic.cpp
===
--- llvm/lib/Transforms/Scalar/LowerExpectIntrinsic.cpp
+++ llvm/lib/Transforms/Scalar/LowerExpectIntrinsic.cpp
@@ -54,13 +54,32 @@
 "unlikely-branch-weight", cl::Hidden, cl::init(1),
 cl::desc("We

[PATCH] D79830: Add support of __builtin_expect_with_probability

[Zhi Zhuang via Phabricator via cfe-commits]

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 lines) when generating patch or show the 
whole function? Thank you!


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
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D79830: Add support of __builtin_expect_with_probability

[Zhi Zhuang via Phabricator via cfe-commits]

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

Files:
  clang/include/clang/Basic/Builtins.def
  clang/include/clang/Basic/DiagnosticSemaKinds.td
  clang/lib/CodeGen/CGBuiltin.cpp
  clang/lib/Sema/SemaChecking.cpp
  clang/test/CodeGen/builtin-expect-with-probability-switch.c
  clang/test/CodeGen/builtin-expect-with-probability.c
  llvm/include/llvm/IR/Intrinsics.td
  llvm/lib/Transforms/Scalar/LowerExpectIntrinsic.cpp

Index: llvm/lib/Transforms/Scalar/LowerExpectIntrinsic.cpp
===
--- llvm/lib/Transforms/Scalar/LowerExpectIntrinsic.cpp
+++ llvm/lib/Transforms/Scalar/LowerExpectIntrinsic.cpp
@@ -55,13 +55,32 @@
 "unlikely-branch-weight", cl::Hidden, cl::init(1),
 cl::desc("Weight of the branch unlikely to be taken (default = 1)"));
 
+std::pair setBranchWeight(Intrinsic::ID IntrinsicID,
+  CallInst *CI, int BranchCount) {
+  if (IntrinsicID == Intrinsic::expect) {
+// __builtin_expect
+return {LikelyBranchWeight, UnlikelyBranchWeight};
+  } else {
+// __builtin_expect_with_probability
+assert(CI->getNumOperands() >= 3 &&
+   "expect with probability must have 3 arguments");
+ConstantFP *Confidence = dyn_cast(CI->getArgOperand(2));
+double TrueProb = Confidence->getValueAPF().convertToDouble();
+double FalseProb = (1.0 - TrueProb) / (BranchCount - 1);
+uint32_t LikelyBW = ceil((TrueProb * (double)(INT32_MAX - 1)) + 1.0);
+uint32_t UnlikelyBW = ceil((FalseProb * (double)(INT32_MAX - 1)) + 1.0);
+return {LikelyBW, UnlikelyBW};
+  }
+}
+
 static bool handleSwitchExpect(SwitchInst &SI) {
   CallInst *CI = dyn_cast(SI.getCondition());
   if (!CI)
 return false;
 
   Function *Fn = CI->getCalledFunction();
-  if (!Fn || Fn->getIntrinsicID() != Intrinsic::expect)
+  if (!Fn || (Fn->getIntrinsicID() != Intrinsic::expect &&
+  Fn->getIntrinsicID() != Intrinsic::expect_with_probability))
 return false;
 
   Value *ArgValue = CI->getArgOperand(0);
@@ -71,15 +90,20 @@
 
   SwitchInst::CaseHandle Case = *SI.findCaseValue(ExpectedValue);
   unsigned n = SI.getNumCases(); // +1 for default case.
-  SmallVector Weights(n + 1, UnlikelyBranchWeight);
+  std::pair WeightNums =
+  setBranchWeight(Fn->getIntrinsicID(), CI, n + 1);
+  uint32_t LikelyBranchWeightVal = WeightNums.first;
+  uint32_t UnlikelyBranchWeightVal = WeightNums.second;
+
+  SmallVector Weights(n + 1, UnlikelyBranchWeightVal);
 
   uint64_t Index = (Case == *SI.case_default()) ? 0 : Case.getCaseIndex() + 1;
-  Weights[Index] = LikelyBranchWeight;
+  Weights[Index] = LikelyBranchWeightVal;
 
-  SI.setMetadata(
-  LLVMContext::MD_misexpect,
-  MDBuilder(CI->getContext())
-  .createMisExpect(Index, LikelyBranchWeight, UnlikelyBranchWeight));
+  SI.setMetadata(LLVMContext::MD_misexpect,
+ MDBuilder(CI->getContext())
+ .createMisExpect(Index, LikelyBranchWeightVal,
+  UnlikelyBranchWeightVal));
 
   SI.setCondition(ArgValue);
   misexpect::checkFrontendInstrumentation(SI);
@@ -223,15 +247,19 @@
 return true;
   return false;
 };
+std::pair WeightNums = setBranchWeight(
+Expect->getCalledFunction()->getIntrinsicID(), Expect, 2);
+uint32_t LikelyBranchWeightVal = WeightNums.first;
+uint32_t UnlikelyBranchWeightVal = WeightNums.second;
 
 if (IsOpndComingFromSuccessor(BI->getSuccessor(1)))
-  BI->setMetadata(
-  LLVMContext::MD_prof,
-  MDB.createBranchWeights(LikelyBranchWeight, UnlikelyBranchWeight));
+  BI->setMetadata(LLVMContext::MD_prof,
+  MDB.createBranchWeights(LikelyBranchWeightVal,
+  UnlikelyBranchWeightVal));
 else if (IsOpndComingFromSuccessor(BI->getSuccessor(0)))
-  BI->setMetadata(
-  LLVMContext::MD_prof,
-  MDB.createBranchWeights(UnlikelyBranchWeight, LikelyBranchWeight));
+  BI->setMetadata(LLVMContext::MD_prof,
+  MDB.createBranchWeights(UnlikelyBranchWeightVal,
+  LikelyBranchWeightVal));
   }
 }
 
@@ -277,7 +305,8 @@
   }
 
   Function *Fn = CI->getCalledFunction();
-  if (!Fn || Fn->getIntrinsicID() != Intrinsic::expect)
+  if (!Fn || (Fn->getIntrinsicID() != Intrinsic::expect &&
+  Fn->getIntrinsicID() != Intrinsic::expect_with_probability))
 return false;
 
   Value *ArgValue = CI->getArgOperand(0);
@@ -289,13 +318,22 @@
   MDNode *Node;
   MDNode *ExpNode;
 
+  std::pair WeightNums =
+  setBranchWeight(Fn->getIntrinsicID(), CI, 2);
+  uint32_t LikelyBranchWeightVal = WeightNums.first;
+

[PATCH] D79830: Add support of __builtin_expect_with_probability

[Zhi Zhuang via Phabricator via cfe-commits]

LukeZhuang added a comment.

In D79830#2034796 , @lebedev.ri wrote:

> 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 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!
>
>
> `git diff -p -U9`


Thanks! It is updated in lastest revision


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

https://reviews.llvm.org/D79830



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D79830: Add support of __builtin_expect_with_probability

[Zhi Zhuang via Phabricator via cfe-commits]

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(),

erichkeane wrote:
> This check should be able to be done during Sema.
fixed in the lastest change



Comment at: clang/lib/CodeGen/CGBuiltin.cpp:2056
+  if (prob < 0.0 || prob > 1.0) {
+CGM.Error(E->getArg(2)->getLocStart(),
+  "probability of __builtin_expect_with_probability is "

erichkeane wrote:
> Same as above, we shouldn't do this during Codegen.
fixed in the lastest change


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

https://reviews.llvm.org/D79830



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D79830: Add support of __builtin_expect_with_probability

[Zhi Zhuang via Phabricator via cfe-commits]

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

rsmith wrote:
> 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.)
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? 



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

rsmith wrote:
> 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?
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?


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

https://reviews.llvm.org/D79830



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D79830: Add support of __builtin_expect_with_probability

[Zhi Zhuang via Phabricator via cfe-commits]

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 wrote:
> > 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?
> 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?
Hi, and I didn't find other builtin functions ever handle case like this, is 
there any example I could refer to?


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

https://reviews.llvm.org/D79830



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D79830: Add support of __builtin_expect_with_probability

[Zhi Zhuang via Phabricator via cfe-commits]

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, "vvC*.", "nc")

rsmith wrote:
> 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.
Seems there is currently no tag for constant floating-point



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

rsmith wrote:
> LukeZhuang wrote:
> > rsmith wrote:
> > > 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.)
> > 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? 
> `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.
Fixed in lastest patch



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

rsmith wrote:
> LukeZhuang wrote:
> > LukeZhuang wrote:
> > > rsmith wrote:
> > > > 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?
> > > 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?
> > Hi, and I didn't find other builtin functions ever handle case like this, 
> > is there any example I could refer to?
> 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.
fixed in lastest patch



Comment at: clang/lib/Sema/SemaChecking.cpp:1806
+if (ProbArg->EvaluateAsFloat(Confidence, Context)) {
+  double P = Confidence.convertToDouble();
+  if (P < 0.0 || P > 1.0) {

rsmith wrote:
> 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?
fixed in lastest patch



Comment at: clang/lib/Sema/SemaChecking.cpp:1807
+  double P = Confidence.convertToDouble();
+  if (P < 0.0 || P > 1.0) {
+Diag(ProbArg->getLocStart(), diag::err_probability_out_of_range)

rsmith wrote:
> This does not disallow NaN values. I think you want `if (!(P >= 0.0 && P <= 
> 1.0))` or equivalent instead.
fixed in lastest patch



Comment at: clang/lib/Sema/SemaC

[PATCH] D79830: Add support of __builtin_expect_with_probability

[Zhi Zhuang via Phabricator via cfe-commits]

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 argument before 
passing it


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

https://reviews.llvm.org/D79830

Files:
  clang/include/clang/Basic/Builtins.def
  clang/include/clang/Basic/DiagnosticSemaKinds.td
  clang/lib/CodeGen/CGBuiltin.cpp
  clang/lib/Sema/SemaChecking.cpp
  clang/test/CodeGen/builtin-expect-with-probability-switch.c
  clang/test/CodeGen/builtin-expect-with-probability.c
  llvm/docs/BranchWeightMetadata.rst
  llvm/include/llvm/IR/Intrinsics.td
  llvm/lib/Transforms/Scalar/LowerExpectIntrinsic.cpp

Index: llvm/lib/Transforms/Scalar/LowerExpectIntrinsic.cpp
===
--- llvm/lib/Transforms/Scalar/LowerExpectIntrinsic.cpp
+++ llvm/lib/Transforms/Scalar/LowerExpectIntrinsic.cpp
@@ -55,13 +55,32 @@
 "unlikely-branch-weight", cl::Hidden, cl::init(1),
 cl::desc("Weight of the branch unlikely to be taken (default = 1)"));
 
+std::pair setBranchWeight(Intrinsic::ID IntrinsicID,
+  CallInst *CI, int BranchCount) {
+  if (IntrinsicID == Intrinsic::expect) {
+// __builtin_expect
+return {LikelyBranchWeight, UnlikelyBranchWeight};
+  } else {
+// __builtin_expect_with_probability
+assert(CI->getNumOperands() >= 3 &&
+   "expect with probability must have 3 arguments");
+ConstantFP *Confidence = dyn_cast(CI->getArgOperand(2));
+double TrueProb = Confidence->getValueAPF().convertToDouble();
+double FalseProb = (1.0 - TrueProb) / (BranchCount - 1);
+uint32_t LikelyBW = ceil((TrueProb * (double)(INT32_MAX - 1)) + 1.0);
+uint32_t UnlikelyBW = ceil((FalseProb * (double)(INT32_MAX - 1)) + 1.0);
+return {LikelyBW, UnlikelyBW};
+  }
+}
+
 static bool handleSwitchExpect(SwitchInst &SI) {
   CallInst *CI = dyn_cast(SI.getCondition());
   if (!CI)
 return false;
 
   Function *Fn = CI->getCalledFunction();
-  if (!Fn || Fn->getIntrinsicID() != Intrinsic::expect)
+  if (!Fn || (Fn->getIntrinsicID() != Intrinsic::expect &&
+  Fn->getIntrinsicID() != Intrinsic::expect_with_probability))
 return false;
 
   Value *ArgValue = CI->getArgOperand(0);
@@ -71,15 +90,20 @@
 
   SwitchInst::CaseHandle Case = *SI.findCaseValue(ExpectedValue);
   unsigned n = SI.getNumCases(); // +1 for default case.
-  SmallVector Weights(n + 1, UnlikelyBranchWeight);
+  std::pair WeightNums =
+  setBranchWeight(Fn->getIntrinsicID(), CI, n + 1);
+  uint32_t LikelyBranchWeightVal = WeightNums.first;
+  uint32_t UnlikelyBranchWeightVal = WeightNums.second;
+
+  SmallVector Weights(n + 1, UnlikelyBranchWeightVal);
 
   uint64_t Index = (Case == *SI.case_default()) ? 0 : Case.getCaseIndex() + 1;
-  Weights[Index] = LikelyBranchWeight;
+  Weights[Index] = LikelyBranchWeightVal;
 
-  SI.setMetadata(
-  LLVMContext::MD_misexpect,
-  MDBuilder(CI->getContext())
-  .createMisExpect(Index, LikelyBranchWeight, UnlikelyBranchWeight));
+  SI.setMetadata(LLVMContext::MD_misexpect,
+ MDBuilder(CI->getContext())
+ .createMisExpect(Index, LikelyBranchWeightVal,
+  UnlikelyBranchWeightVal));
 
   SI.setCondition(ArgValue);
   misexpect::checkFrontendInstrumentation(SI);
@@ -223,15 +247,19 @@
 return true;
   return false;
 };
+std::pair WeightNums = setBranchWeight(
+Expect->getCalledFunction()->getIntrinsicID(), Expect, 2);
+uint32_t LikelyBranchWeightVal = WeightNums.first;
+uint32_t UnlikelyBranchWeightVal = WeightNums.second;
 
 if (IsOpndComingFromSuccessor(BI->getSuccessor(1)))
-  BI->setMetadata(
-  LLVMContext::MD_prof,
-  MDB.createBranchWeights(LikelyBranchWeight, UnlikelyBranchWeight));
+  BI->setMetadata(LLVMContext::MD_prof,
+  MDB.createBranchWeights(LikelyBranchWeightVal,
+  UnlikelyBranchWeightVal));
 else if (IsOpndComingFromSuccessor(BI->getSuccessor(0)))
-  BI->setMetadata(
-  LLVMContext::MD_prof,
-  MDB.createBranchWeights(UnlikelyBranchWeight, LikelyBranchWeight));
+  BI->setMetadata(LLVMContext::MD_prof,
+  MDB.createBranchWeights(UnlikelyBranchWeightVal,
+  LikelyBranchWeightVal));
   }
 }
 
@@ -277,7 +305,8 @@
   }
 
   Function *Fn = CI->getCalledFunction();
-  if (!Fn || Fn->getIntrinsicID() != Intrinsic::expect)
+  if (!Fn || (Fn->getIntrinsicID() != Intrinsic::expect &&
+  Fn->getIntrinsicID() != Intrinsic::expect_with_probability))
 return false;
 
   Value *ArgValue = CI->getArgOperand(0);
@@ -289,13 +318,22 @@
   MDNode *Node