[PATCH] D24166: [Profile] Lower expect intrinsic properly for select instruction

[Sanjay Patel via llvm-commits]

spatel added inline comments.

================
Comment at: lib/Transforms/Scalar/LowerExpectIntrinsic.cpp:86
@@ -85,5 +85,3 @@
 
-static bool handleBranchExpect(BranchInst &BI) {
-  if (BI.isUnconditional())
-    return false;
+template <class BrSelInst> static bool handleBrSelExpect(BrSelInst &BI) {
 
----------------
Neat trick. :)
But can you add a comment and/or rename BI, so it is clear that this isn't strictly for a 'BI' (BranchInst) any more?

================
Comment at: lib/Transforms/Scalar/LowerExpectIntrinsic.cpp:159
@@ -153,2 +158,3 @@
 
     // Remove llvm.expect intrinsics.
+    for (BasicBlock::reverse_iterator BI = BB.rbegin(), BE = BB.rend(); BI != BE;) {
----------------
Fix comment to indicate why we are using reverse iterator (and that we're also handling selects in this loop).

================
Comment at: lib/Transforms/Scalar/LowerExpectIntrinsic.cpp:160
@@ -154,4 +159,3 @@
     // Remove llvm.expect intrinsics.
-    for (BasicBlock::iterator BI = BB.begin(), BE = BB.end(); BI != BE;) {
-      CallInst *CI = dyn_cast<CallInst>(BI++);
-      if (!CI)
+    for (BasicBlock::reverse_iterator BI = BB.rbegin(), BE = BB.rend(); BI != BE;) {
+      Instruction *Inst = &*BI++;
----------------
Use 'auto' to avoid 80-col ugliness?

================
Comment at: test/Transforms/LowerExpectIntrinsic/basic.ll:276-287
@@ -275,1 +275,14 @@
 
+; CHECK-LABEL: @test10(
+define i32 @test10(i32, i32, i32) #0 {
+  %4 = and i32 %0, 3
+  %5 = icmp eq i32 %4, 1
+  %6 = zext i1 %5 to i64
+  %7 = call i64 @llvm.expect.i64(i64 %6, i64 0)
+  %8 = icmp ne i64 %7, 0
+  %9 = select i1 %8, i32 %1, i32 %2
+; CHECK: select{{.*}}, !prof !1
+  ret i32 %9
+}
+
+
----------------
Minimize test?

  define i32 @test10(i64 %t6) {
    %t7 = call i64 @llvm.expect.i64(i64 %t6, i64 0)
    %t8 = icmp ne i64 %t7, 0
    %t9 = select i1 %t8, i32 42, i32 43
    ret i32 %t9
  }



https://reviews.llvm.org/D24166