[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:279-282
+        // Before erasing the phi, walk backward to find
+        // phi that define builin_expect's first arg, and
+        // infer branch probability:
+        handlePhiDef(CI);
----------------
davidxl wrote:
> chandlerc wrote:
> > This doesn't erase a PHI node though... Maybe this comment just needs updating?
> It is referring to the removal after this call.
Ok, but you've written this comment right above code that erases the *call* and not any phi node, so it seems a bit confusing to write the comment this way. And erasing the phi node doesn't seem as relevant as erasing the call... 


================
Comment at: lib/Transforms/Scalar/LowerExpectIntrinsic.cpp:97
+  Value &Arg = *Expect->getArgOperand(0);
+  ConstantInt *ExpectedValue = dyn_cast<ConstantInt>(Expect->getArgOperand(1));
+  PHINode *PhiDef = nullptr;
----------------
You don't check whether this is null... Either add a check or use `cast`?


================
Comment at: lib/Transforms/Scalar/LowerExpectIntrinsic.cpp:100
+  bool ValueInverted = false;
+  // Walk up in backward a list of instructions that
+  // have 'copy' semantics by 'stripping' the copies
----------------
nit: Some vertical whitespace before this comment block would make it easier to read IMO.


================
Comment at: lib/Transforms/Scalar/LowerExpectIntrinsic.cpp:121-136
+    if (BinaryOperator *BinOp = dyn_cast<BinaryOperator>(V)) {
+      Instruction::BinaryOps Opcode = BinOp->getOpcode();
+      if (Opcode == Instruction::Xor) {
+        Value *Opnd0 = BinOp->getOperand(0);
+        Value *Opnd1 = BinOp->getOperand(1);
+
+        if (ConstantInt *CInt = dyn_cast<ConstantInt>(Opnd1)) {
----------------
I think all of this can now be re-written using early return though, and I think that plus removing some unnecessary variables helps make the rest of the loop more readable:

  BinaryOperator *BinOp = dyn_cast<BinaryOperator>(V)
  if (!BinOp || BinOp->getOpcode() != Instruction::Xor)
    return;
    
  ConstantInt *COperand = dyn_cast<ConstantInt>(BinOp->getOperand(1));
  if (!COperand)
    return;
  
  if (!COperand->isZero())
    ValueInverted = !ValueInverted;
  
  V = BinOp->getOperand(0);

This also makes it more clear (at least to me) that there is a bug here. When the value isn't an `i1` we can't assume that all non-zero xors negate. I think this will need to check that the constant operand is specifically either zero or all ones. Which ends up looking like:

  ConstantInt *COperand = dyn_cast<ConstantInt>(BinOp->getOperand(1));
  if (!COperand)
    return;
  
  if (!COperand->isZero()) {
    // If this isn't a no-op copy, check that it is exactly a negation.
    if (!COperand->isMinusOne())
      return;

    ValueInverted = !ValueInverted;
  }
  
  V = BinOp->getOperand(0);



================
Comment at: lib/Transforms/Scalar/LowerExpectIntrinsic.cpp:140
+  PhiDef = dyn_cast<PHINode>(V);
+
+  if (!PhiDef)
----------------
nit: I actually prefer the dyn_cast line right before the condition. Also, declare this here with `auto *`?


================
Comment at: lib/Transforms/Scalar/LowerExpectIntrinsic.cpp:144-145
+
+  bool IsPhiValueOne = ((!ValueInverted && !ExpectedValue->isZero()) ||
+                        (ValueInverted && ExpectedValue->isZero()));
+
----------------
How do you want this to handle non-i1 expect calls?


================
Comment at: lib/Transforms/Scalar/LowerExpectIntrinsic.cpp:149
+  // i's incoming block.
+  auto GetDomConditional = [=](unsigned i) -> BranchInst * {
+    BasicBlock *BB = PhiDef->getIncomingBlock(i);
----------------
Capturing by copy is pretty expensive. Given that this is called only from within this function, capturing by reference seems a bit more appropriate?


================
Comment at: lib/Transforms/Scalar/LowerExpectIntrinsic.cpp:181-186
+    // Figuring out which outgoing edge from BR that leads
+    // to the PhiDef's parent block. If the CFG is triangular
+    // shaped and the the operand's incoming block is BR's parent
+    // block itself, Phi's parent BB is the direct successor
+    // of BR's parent; Otherwise check the incoming block of the
+    // operand to see it if is one BR's successors:
----------------
Reading this loop, I felt like this could end up setting the same metadata on a branch multiple times...

But reading this I feel like this isn't actually the case. But I'm also not exactly sure how this is working. I mean, I understand at a high level the cases you're trying to handle (I've read the test cases), but it really isn't clear how *this* code is effectively doing that.

Maybe an ascii-art diagram of the two cases you're trying to handle here would help? Maybe specifically talking about why we're looping over all of the phi's operands? (I had asked for the latter already...)


https://reviews.llvm.org/D33164