[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] D79830: Add support of __builtin_expect_with_probability

[Erich Keane via Phabricator via cfe-commits]

erichkeane added a comment.

@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.


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

[Erich Keane via Phabricator via cfe-commits]

erichkeane added a comment.

In D79830#2107404 , @LukeZhuang wrote:

> Thank you very much!


Turns out when I was validating my patch, someone had beaten me to it, it 
should be fixed, it just wasn't me :)


Repository:
  rG LLVM Github Monorepo

CHANGES 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

[Erich Keane via Phabricator via cfe-commits]

erichkeane added a comment.

I'll get this fixed, thanks both of you for letting me know.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D79830



___
cfe-commits 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

[Aaron Enye Shi via Phabricator via cfe-commits]

ashi1 added subscribers: yaxunl, ashi1.
ashi1 added a comment.

Hi, I am getting a compiler error due to this patch. I am using cmake command: 
cmake ../llvm -DCMAKE_BUILD_TYPE=Release 
-DLLVM_ENABLE_PROJECTS="clang;lld;clang-tools-extra" 
-DLLVM_TARGETS_TO_BUILD="AMDGPU;X86" -DLLVM_ENABLE_ASSERTIONS=1 . Could you 
please take a look?

**Error:**
/root/llvm-project/llvm/lib/Transforms/Scalar/LowerExpectIntrinsic.cpp: In 
function 'std::tuple 
getBranchWeight(llvm::Intrinsic::ID, llvm::CallInst*, int)':
/root/llvm-project/llvm/lib/Transforms/Scalar/LowerExpectIntrinsic.cpp:62:53: 
error: converting to 'std::tuple' from initializer 
list would use explicit constructor 'constexpr std::tuple<_T1, 
_T2>::tuple(_U1&&, _U2&&) [with _U1 = llvm::opt&; _U2 = 
llvm::opt&;  = void; _T1 = unsigned int; 
_T2 = unsigned int]'

  return {LikelyBranchWeight, UnlikelyBranchWeight};
  ^

/root/llvm-project/llvm/lib/Transforms/Scalar/LowerExpectIntrinsic.cpp:74:33: 
error: converting to 'std::tuple' from initializer 
list would use explicit constructor 'constexpr std::tuple<_T1, 
_T2>::tuple(_U1&&, _U2&&) [with _U1 = unsigned int&; _U2 = unsigned int&; 
 = void; _T1 = unsigned int; _T2 = unsigned int]'

  return {LikelyBW, UnlikelyBW};
  ^


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

[Lei Zhang via Phabricator via cfe-commits]

antiagainst added inline comments.



Comment at: llvm/lib/Transforms/Scalar/LowerExpectIntrinsic.cpp:62
+// __builtin_expect
+return {LikelyBranchWeight, UnlikelyBranchWeight};
+  } else {

FYI: this breaks GCC5: 
https://buildkite.com/mlir/mlir-core/builds/5841#4a7c245b-9841-44aa-a82a-97686b04b672


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

[Erich Keane via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rG37fb86030127: Add support of 
__builtin_expect_with_probability (authored by LukeZhuang, committed by 
erichkeane).

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  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 

[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.

@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] 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 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

[Dávid Bolvanský via Phabricator via cfe-commits]

xbolva00 added inline comments.



Comment at: clang/lib/CodeGen/CGBuiltin.cpp:2216
+// it could have side-effects.
+if (CGM.getCodeGenOpts().OptimizationLevel == 0)
+  return RValue::get(ArgValue);

Fix comment and remove Probability? 


CHANGES SINCE 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 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

[Erich Keane via Phabricator via cfe-commits]

erichkeane accepted this revision.
erichkeane added a comment.

Please fix the 'nit' when committing.




Comment at: clang/lib/CodeGen/CGBuiltin.cpp:2207
+llvm::RoundingMode::Dynamic, );
+(void)EvalSucceed;
+llvm::Type *Ty = ConvertType(ProbArg->getType());

Nit, move this cast next to the assert, its a common pattern in LLVM, so moving 
it away makes its purpose pretty unknown.


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.

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

[David Li via Phabricator via cfe-commits]

davidxl accepted this revision.
davidxl added a comment.
This revision is now accepted and ready to land.

LLVM side of change LGTM.


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

https://reviews.llvm.org/D79830



___
cfe-commits 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 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

[David Li via Phabricator via cfe-commits]

davidxl added inline comments.



Comment at: llvm/lib/Transforms/Scalar/LowerExpectIntrinsic.cpp:58
 
+std::pair setBranchWeight(Intrinsic::ID IntrinsicID,
+  CallInst *CI, int BranchCount) {

nit: change name to  getBranchWeight or computeBranchWeight 



Comment at: llvm/lib/Transforms/Scalar/LowerExpectIntrinsic.cpp:69
+double TrueProb = Confidence->getValueAPF().convertToDouble();
+double FalseProb = (1.0 - TrueProb) / (BranchCount - 1);
+uint32_t LikelyBW = ceil((TrueProb * (double)(INT32_MAX - 1)) + 1.0);

assert TrueProb is in [0,1]



Comment at: llvm/lib/Transforms/Scalar/LowerExpectIntrinsic.cpp:96
+  uint32_t LikelyBranchWeightVal = WeightNums.first;
+  uint32_t UnlikelyBranchWeightVal = WeightNums.second;
+

simpler to use tuple and tie 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 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: 

[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 ) {
   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 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 ) {
   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,
+ 

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

[David Li via Phabricator via cfe-commits]

davidxl added a comment.

Can you add some tests at the LLVM side?


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

https://reviews.llvm.org/D79830



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

[PATCH] D79830: Add support of __builtin_expect_with_probability

[Richard Smith - zygoloid via Phabricator via cfe-commits]

rsmith added inline comments.



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

I think "probability argument to" rather than "probability of" would be clearer.



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

erichkeane wrote:
> erichkeane wrote:
> > I believe in EvaluateAsFloat you need to pass  'allow side effects', 
> > because by default it does not.
> Thinking further, Since it is with constant-expressions, we might consider 
> disallowing side effects.
(This is marked as done, but `SE_AllowSideEffects` is still passed above.)



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)

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



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]}}

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.



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.
 

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.



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.

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.


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/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 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 ) {
   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

[Roman Lebedev via Phabricator via cfe-commits]

lebedev.ri added inline comments.



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

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?



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

Do these tests actually pass?


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.

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 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 ) {
   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 = 

[PATCH] D79830: Add support of __builtin_expect_with_probability

[Erich Keane via Phabricator via cfe-commits]

erichkeane added a comment.

1 more nit that I saw (don't use an 'else' after an 'if' branch that returns), 
but otherwise I think this is good from the CFE perspective.  Someone better 
understanding of LLVM needs to look at the rest though.




Comment at: clang/lib/Sema/SemaChecking.cpp:1816
+  return ExprError();
+} else {
+  llvm::APFloat Probability = Eval.Val.getFloat();

You don't need the else, since the above branch returns.


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 ) {
   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 

[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

[Erich Keane via Phabricator via cfe-commits]

erichkeane added a comment.

Additionally, it seems you're missing SEMA tests.  Please add those.  Those 
tests should also make sure that the diagnostics still work with dependent 
values.




Comment at: clang/include/clang/Basic/Builtins.def:567
 BUILTIN(__builtin_expect, "LiLiLi"   , "nc")
+BUILTIN(__builtin_expect_with_probability, "LiLiLid"   , "nc")
 BUILTIN(__builtin_prefetch, "vvC*.", "nc")

The spaces after the first parameter are odd/unnecessary.  I suspect it is in 
the line above because it was used to do alignment in the past.  Please don't 
bother putting them in this new line.



Comment at: clang/lib/CodeGen/CGBuiltin.cpp:2197
+const Expr *ProbArg = E->getArg(2);
+bool EvalSucceed = ProbArg->EvaluateAsFloat(Probability,
+CGM.getContext(), Expr::SideEffectsKind::SE_AllowSideEffects);

This is going to give an unused variable warning in release mode, since assert 
is a macro. You'll have to cast EvalSucceeded to void after the assert.



Comment at: clang/lib/CodeGen/CGBuiltin.cpp:2199
+CGM.getContext(), Expr::SideEffectsKind::SE_AllowSideEffects);
+assert(EvalSucceed == true);
+llvm::Type *Ty = ConvertType(ProbArg->getType());

First, don't do ==true here.  Second, add a string to the assert (see other 
assert uses).


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 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 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 ) {
   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;
   

[PATCH] D79830: Add support of __builtin_expect_with_probability

[Erich Keane via Phabricator via cfe-commits]

erichkeane added a comment.

FYI: I'm more of a clang contributor, so I'm unable to review the LLVM code, 
hopefully someone will come along who can check on that.




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

LukeZhuang wrote:
> 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 , const ASTContext ,
> > > 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!
Right, exactly.  This code path should end up being evaluated 2x in the case of 
an instantiation, the first time it is with the dependent placeholder types.  
The second time it will be instantiated.  SO if you cannot validate the rules 
against dependent values, you just assume it would work and let the 2nd pass 
catch it.

Include some tests to make sure you catch this right and that this is usable in 
templates.



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

LukeZhuang wrote:
> 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.
Ok, thanks for checking. I was unsure, but I presumed that the implicit 
type-conversion based on the builtin definition would have caused that to 
happen.  


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 , const ASTContext ,
> > 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

[Erich Keane via Phabricator via cfe-commits]

erichkeane added inline comments.



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

erichkeane wrote:
> I believe in EvaluateAsFloat you need to pass  'allow side effects', because 
> by default it does not.
Thinking further, Since it is with constant-expressions, we might consider 
disallowing side effects.



Comment at: clang/lib/CodeGen/CGBuiltin.cpp:2204
+if (CGM.getCodeGenOpts().OptimizationLevel == 0)
+  return RValue::get(ArgValue);
+

LukeZhuang wrote:
> 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!
You're right here, I'd read the comment and skipped that ArgValue was evaluated 
above.



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

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 , const ASTContext ,
> 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.


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: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 , const ASTContext ,
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

[Erich Keane via Phabricator via cfe-commits]

erichkeane added inline comments.



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

You likely need to assert on the return value (as Richard suggested).



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

I believe in EvaluateAsFloat you need to pass  'allow side effects', because by 
default it does not.



Comment at: clang/lib/CodeGen/CGBuiltin.cpp:2204
+if (CGM.getCodeGenOpts().OptimizationLevel == 0)
+  return RValue::get(ArgValue);
+

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.



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

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
}




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

BTW, this check should be valid (ensuring its a float), since normal 
conversions should happen as a part of the function call.


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 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 ) {
   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;
 

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

[PATCH] D79830: Add support of __builtin_expect_with_probability

[Richard Smith - zygoloid via Phabricator via cfe-commits]

rsmith added inline comments.



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

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.



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



Comment at: clang/lib/Sema/SemaChecking.cpp:1813-1814
+} else {
+  Diag(ProbArg->getLocStart(), diag::err_probability_not_constant_float)
+  << ProbArg->getSourceRange();
+  return ExprError();

`EvaluateAsConstantExpr` will give you some notes to attach to this diagnostic 
to explain why the expression is non-constant.


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

[Richard Smith - zygoloid via Phabricator via cfe-commits]

rsmith added inline comments.



Comment at: clang/include/clang/Basic/Builtins.def:567
 BUILTIN(__builtin_expect, "LiLiLi"   , "nc")
+BUILTIN(__builtin_expect_with_probability, "LiLiLid"   , "nc")
 BUILTIN(__builtin_prefetch, "vvC*.", "nc")

I assume we don't have 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.



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

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.)



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

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?



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

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?



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)

This does not disallow NaN values. I think you want `if (!(P >= 0.0 && P <= 
1.0))` or equivalent instead.


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#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 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 ) {
   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

[Erich Keane via Phabricator via cfe-commits]

erichkeane added a comment.

In D79830#2034779 , @LukeZhuang wrote:

> In D79830#2033367 , @lebedev.ri 
> wrote:
>
> > Thanks for working on this.
> >  Please upload patch with full context.
>
>
> Hi, sorry but I'm 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!


See https://llvm.org/docs/Phabricator.html particularly 
https://llvm.org/docs/Phabricator.html#requesting-a-review-via-the-web-interface


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

[Roman Lebedev via Phabricator via cfe-commits]

lebedev.ri added a comment.

In D79830#2034779 , @LukeZhuang wrote:

> In D79830#2033367 , @lebedev.ri 
> wrote:
>
> > Thanks for working on this.
> >  Please upload patch with full context.
>
>
> Hi, sorry but I'm 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`


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

[Dávid Bolvanský via Phabricator via cfe-commits]

xbolva00 added a comment.

In D79830#2034380 , @RKSimon wrote:

> Add a description in our documentation?


And Ideally add a note to release notes too..


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  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

[Erich Keane via Phabricator via cfe-commits]

erichkeane added a comment.

In D79830#2034400 , @davidxl wrote:

> Is it possible to overload __builtin_expect(..)?


No OP, but...
First, Overloading builtins is a bit of a pain.  You end up having to do custom 
type checking.
Second, GCC already made the decision to do a separate name.  I'd want us to 
match them unless we have a really good reason not to.


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

[David Li via Phabricator via cfe-commits]

davidxl added a comment.

Is it possible to overload __builtin_expect(..)?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D79830



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

[PATCH] D79830: Add support of __builtin_expect_with_probability

[Simon Pilgrim via Phabricator via cfe-commits]

RKSimon added a comment.

Add a description in our documentation?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D79830



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

[PATCH] D79830: Add support of __builtin_expect_with_probability

[Erich Keane via Phabricator via cfe-commits]

erichkeane added a comment.

Type checking/value checking for this should happen during Sema, not codegen.




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

This check should be able to be done during Sema.



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 "

Same as above, we shouldn't do this during Codegen.


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

[Roman Lebedev via Phabricator via cfe-commits]

lebedev.ri added a reviewer: erichkeane.
lebedev.ri added a comment.

Thanks for working on this.
Please upload patch with full context.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D79830



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