[PATCH] D33164: [Profile[ Enhance expect lowering to handle correlated branches

[Chandler Carruth via Phabricator via llvm-commits]

chandlerc added a comment.

some of minor nits a a small thing left... thanks for the handling and the nice test cases around larger integers!



================
Comment at: lib/Transforms/Scalar/LowerExpectIntrinsic.cpp:96
+  ConstantInt *ExpectedValue = cast<ConstantInt>(Expect->getArgOperand(1));
+  uint64_t ExpectedPhiValue = ExpectedValue->getZExtValue();
+
----------------
You'll need to use the APInt so this can handle larger than 64-bit integers (or you'll need to bail out when the integer requires more than 64-bits). The APInt code should be pretty straight forward here though.

(also, add a test case (regardless of which direction you go) with 73 bit integer or some such?)


================
Comment at: lib/Transforms/Scalar/LowerExpectIntrinsic.cpp:116-117
+      V = ZExt->getOperand(0);
+      ExpectedPhiValue &=
+          GetBitMask(ZExt->getOperand(0)->getType()->getIntegerBitWidth());
+      continue;
----------------
`MathExtras.h` provides `maskTrailingOnes` which implements this. However, if you end up moving to `APInt` it has a pretty rich interface for doing these kinds of operations. You may be able to just call `.zext` on it to model the zext.


================
Comment at: lib/Transforms/Scalar/LowerExpectIntrinsic.cpp:122-124
+      ExpectedPhiValue &=
+          GetBitMask(SExt->getOperand(0)->getType()->getIntegerBitWidth());
+      V = SExt->getOperand(0);
----------------
It's a bit surprising that these two statements are in the opposite order as above... But anyways, doesn't this need to be a sign extend of the expected value?


================
Comment at: lib/Transforms/Scalar/LowerExpectIntrinsic.cpp:181-188
+    // There are two situations in which an operand of the PhiDef comes
+    // from a given successor of a branch instruction BI.
+    // 1) When the incoming block of the operand is the successor block;
+    // 2) When the incoming block is BI's enclosing block and the
+    // successor is the PhiDef's enclosing block.
+    //
+    // Returns true if the operand which comes from OpndIncomingBB
----------------
FWIW, this comment now really helps me with the whole thing. Thanks!


================
Comment at: lib/Transforms/Scalar/LowerExpectIntrinsic.cpp:203
+
+    auto *OpndIncomingBB = PhiDef->getIncomingBlock(i);
+    auto *Succ0 = BI->getSuccessor(0);
----------------
Hoist this above the lambda, and thin skip the lambda argument? It doesn't vary between calls.


================
Comment at: lib/Transforms/Scalar/LowerExpectIntrinsic.cpp:204-205
+    auto *OpndIncomingBB = PhiDef->getIncomingBlock(i);
+    auto *Succ0 = BI->getSuccessor(0);
+    auto *Succ1 = BI->getSuccessor(1);
+    if (IsOpndComingFromSuccessor(OpndIncomingBB, Succ1))
----------------
Skip the variables and just inline the successor access?


================
Comment at: test/Transforms/LowerExpectIntrinsic/phi_merge.ll:14-24
+  %tmp = alloca i32, align 4
+  %tmp4 = alloca i32, align 4
+  %tmp5 = alloca i32, align 4
+  %tmp6 = alloca i32, align 4
+  store i32 %arg, i32* %tmp, align 4
+  store i32 %arg1, i32* %tmp4, align 4
+  store i32 %arg2, i32* %tmp5, align 4
----------------
chandlerc wrote:
> chandlerc wrote:
> > chandlerc wrote:
> > > Can you minimize the test cases rather than including all of the boilerplate introduced by Clang?
> > > 
> > > Similarly, no use variadic function calls here now that it is an LLVM IR test case.
> > Can still remove the '(...)' from the call instructions...
> Outstanding comment.
Still outstanding...


================
Comment at: test/Transforms/LowerExpectIntrinsic/phi_merge.ll:119
+  %tmp19 = phi i32 [ 5, %bb10 ], [ 5, %bb ], [ %tmp16, %bb14 ]
+  %tmp23 = sext i32 %tmp19 to i64
+  %tmp24 = call i64 @llvm.expect.i64(i64 %tmp23, i64 4)
----------------
Add a test (somewhere) for `sext` when the incoming value from the Phi node will get is -1? I'd probably use an `i8` so its easy to write `255` as the value and sign extend it but expect that it is still `255`. I think that test case would do the wrong thing currently? (see comment above, but maybe I've misread the code above.) 


================
Comment at: test/Transforms/LowerExpectIntrinsic/phi_merge.ll:270-275
+attributes #0 = { noinline nounwind uwtable }
+attributes #1 = { nounwind readnone }
+
+!llvm.ident = !{!0}
+
+!0 = !{!"clang version 5.0.0 (trunk 302965)"}
----------------
Note that in this test case and all the others you can strip off the attributes and the metadata here.


https://reviews.llvm.org/D33164