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

[Chandler Carruth via Phabricator via llvm-commits]

chandlerc added inline comments.


================
Comment at: lib/Transforms/Scalar/LowerExpectIntrinsic.cpp:86-87
 
+/// Handler routine for PHINode instructions that define
+/// Expect instrinsic's arg values.
+///
----------------
nit: wording might be clearer as:

  Handler for PHINodes that define the value argument to an @llvm.expect call.



================
Comment at: lib/Transforms/Scalar/LowerExpectIntrinsic.cpp:89-90
+///
+/// Handle PHI node which define the value of the the builtin_expect's
+/// argument value: If the operand of the phi has a constant value and
+/// it 'contradicts' with the expected value of phi def, then the
----------------
nit: no need to repeate the summary here. I think you can jump to `If the operand ...`.


================
Comment at: lib/Transforms/Scalar/LowerExpectIntrinsic.cpp:98
+  ConstantInt *ExpectedValue = dyn_cast<ConstantInt>(Expect->getArgOperand(1));
+  assert(ExpectedValue);
+  bool ValueInverted = false;
----------------
If you're just going to assert, you should use `cast` rather than `dyn_cast` -- it does exactly this, it asserts, but it does so more cheaply in non-asserts builds.


================
Comment at: lib/Transforms/Scalar/LowerExpectIntrinsic.cpp:198
+    // of BI's parent; Otherwise check the incoming block of the
+    // operand to see it if is one of BI's successors:
+    //
----------------
nit: ':' -> '.'.


================
Comment at: lib/Transforms/Scalar/LowerExpectIntrinsic.cpp:200-203
+    // Returns true of Phi's operand 'opnd' is coming from 'succ' th
+    // successor of BI:
+    auto IsOpndComingFromSuccessor = [&](int opnd, int succ) {
+      auto *OpndIncomingBB = PhiDef->getIncomingBlock(opnd);
----------------
I think this API is overall a bit confusing because we call it with integer indices, but it captures a specific branch and phi instruction...

Maybe, rather than getting the incoming block inside the lambda, why not pull the computation of `OpndIncomingBB` out of the lambda entirely? It doesn't change between the calls below.

Similarly, I feel like it would be a bit more readable to pass in the successor basic block and call `BI->getSuccessor()` in the call as that avoids documenting the magical 1 and 0 indices.


================
Comment at: lib/Transforms/Scalar/LowerExpectIntrinsic.cpp:202
+    // successor of BI:
+    auto IsOpndComingFromSuccessor = [&](int opnd, int succ) {
+      auto *OpndIncomingBB = PhiDef->getIncomingBlock(opnd);
----------------
`OperandIdx` and `SuccIdx` would be better parameter names here I think.


================
Comment at: lib/Transforms/Scalar/LowerExpectIntrinsic.cpp:318
       if (Fn && Fn->getIntrinsicID() == Intrinsic::expect) {
+        // Before erasing the phi, walk backward to find
+        // phi that define builin_expect's first arg, and
----------------
Again, I think 'Before erasing the phi' is confusing here. This code doesn't erase the phi node. I think this should say 'Before erasing the llvm.expect call, ...' which can then make it easier to refer to the first argument in the rest of the sentence.


================
Comment at: lib/Transforms/Scalar/LowerExpectIntrinsic.cpp:144-145
+
+  bool IsPhiValueOne = ((!ValueInverted && !ExpectedValue->isZero()) ||
+                        (ValueInverted && ExpectedValue->isZero()));
+
----------------
chandlerc wrote:
> davidxl wrote:
> > chandlerc wrote:
> > > How do you want this to handle non-i1 expect calls?
> > As other cases, arbitrary value builtin_expect is not yet properly handled.   One possible use of it is to annotate switch statement, but it is pretty uncommon.
> > 
> > I added a TODO there 
> I don't think a TODO is really enough. We shouldn't put completely wrong probabilities on something just because we don't handle non-i1 expects. I'm fine if you just want to check at the start of this that the type is i1 and return otherwise.
I'm still not seeing any response to my follow-up.


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


https://reviews.llvm.org/D33164