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

[Chandler Carruth via Phabricator via llvm-commits]

chandlerc requested changes to this revision.
chandlerc added inline comments.
This revision now requires changes to proceed.


================
Comment at: lib/Transforms/Scalar/LowerExpectIntrinsic.cpp:86-93
+// 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 corresponding incoming edge
+// of the phi is unlikely to be taken. Using that
+// information, the branch probability info for the
----------------
Please use a doxygen comment, and format it consistently with modern documentation comments in LLVM.


================
Comment at: lib/Transforms/Scalar/LowerExpectIntrinsic.cpp:94
+// originating branch can be inferred.
+void static handlePhiDef(CallInst *Expect) {
+  Value *ArgValue = Expect->getArgOperand(0);
----------------
LLVM always uses `static void` not `void static`.


================
Comment at: lib/Transforms/Scalar/LowerExpectIntrinsic.cpp:94
+// originating branch can be inferred.
+void static handlePhiDef(CallInst *Expect) {
+  Value *ArgValue = Expect->getArgOperand(0);
----------------
chandlerc wrote:
> LLVM always uses `static void` not `void static`.
Use a reference since this cannot be null.


================
Comment at: lib/Transforms/Scalar/LowerExpectIntrinsic.cpp:95
+void static handlePhiDef(CallInst *Expect) {
+  Value *ArgValue = Expect->getArgOperand(0);
+  ConstantInt *ExpectedValue = dyn_cast<ConstantInt>(Expect->getArgOperand(1));
----------------
`ArgValue` seems redundant, just `Arg`?


================
Comment at: lib/Transforms/Scalar/LowerExpectIntrinsic.cpp:99-100
+  bool ValueInverted = false;
+  // walk up the copy chain until phi is reached or
+  // non copy like instruction is reached.
+  Value *V = ArgValue;
----------------
This comment isn't really verbose enough to help th ereader out. Can you use proper prose and expand on it so that folks not familiar with the specific pattern you're thinking of ('copy chain'?) understand what is going on?


================
Comment at: lib/Transforms/Scalar/LowerExpectIntrinsic.cpp:102
+  Value *V = ArgValue;
+  while (true) {
+    if (ZExtInst *ZExt = dyn_cast<ZExtInst>(V)) {
----------------
We write infinite loops more often as 'for (;;)', but I'm not sure this really needs to use that pattern...


================
Comment at: lib/Transforms/Scalar/LowerExpectIntrinsic.cpp:113
+
+        assert(!dyn_cast<ConstantInt>(Opnd0) && "expression not canonicalized");
+        if (ConstantInt *CInt = dyn_cast<ConstantInt>(Opnd1)) {
----------------
You can't assert that IR is in canonical form. You can only assert that it would pass the verifier.

But you don't need to *handle* non-canonical form, you can just fail to add the metadata.


================
Comment at: lib/Transforms/Scalar/LowerExpectIntrinsic.cpp:115-117
+          V = Opnd0;
+          if (CInt->getZExtValue())
+            ValueInverted = !ValueInverted;
----------------
If you just want to test for zero, you can do that directly without zext-ing the constant...


================
Comment at: lib/Transforms/Scalar/LowerExpectIntrinsic.cpp:134-135
+
+  if (!PhiDef)
+    return;
+
----------------
You could just use one variable above (`V` or `Phi`) and write the loop to essentially be:

  // Walk up through the operands until we find a PHI node.
  while (!isa<PHINode>(V)) {
    ...
  }

  auto *PN = dyn_cast<PHINode>(V);
  if (!PN)
    // Nothing to do if we cannot find a PHI node.
    return;


================
Comment at: lib/Transforms/Scalar/LowerExpectIntrinsic.cpp:147-150
+    // CFG may not be simplified yet, walk up one level
+    BB = BB->getSinglePredecessor();
+    if (!BB)
+      return nullptr;
----------------
If the CFG isn't simplified, we need to walk more than one. If it is, we don't need to walk any. Do you have cases where this was necessary?


================
Comment at: lib/Transforms/Scalar/LowerExpectIntrinsic.cpp:157
+
+  // Now walk through all Phi operands:
+  for (unsigned i = 0, e = PhiDef->getNumIncomingValues(); i != e; ++i) {
----------------
This comment isn't really helping me. From the code, I know you're walking the PHI operands, but I don't know why or what you're planning to do with them. 


================
Comment at: lib/Transforms/Scalar/LowerExpectIntrinsic.cpp:176
+
+    // Triangular or diamond shaped:
+    if (PhiDef->getParent() == BR->getSuccessor(1) ||
----------------
I think this is definitely a place where explaining more would help. The comment as is doesn't really help me read the code.


================
Comment at: lib/Transforms/Scalar/LowerExpectIntrinsic.cpp:184-185
+
+    if (Node)
+      BR->setMetadata(LLVMContext::MD_prof, Node);
+  }
----------------
Why not just call `BR->setMetadata` twice above? The variable doesn't seem to save a lot here...


================
Comment at: lib/Transforms/Scalar/LowerExpectIntrinsic.cpp:279-282
+        // Before erasing the phi, walk backward to find
+        // phi that define builin_expect's first arg, and
+        // infer branch probability:
+        handlePhiDef(CI);
----------------
This doesn't erase a PHI node though... Maybe this comment just needs updating?


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


https://reviews.llvm.org/D33164