[PATCH] D33164: [Profile[ Enhance expect lowering to handle correlated branches
Chandler Carruth via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Fri May 19 11:04:32 PDT 2017
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);
----------------
chandlerc wrote:
> 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...
Outstanding comment.
================
Comment at: lib/Transforms/Scalar/LowerExpectIntrinsic.cpp:97
+ Value &Arg = *Expect->getArgOperand(0);
+ ConstantInt *ExpectedValue = dyn_cast<ConstantInt>(Expect->getArgOperand(1));
+ PHINode *PhiDef = nullptr;
----------------
chandlerc wrote:
> You don't check whether this is null... Either add a check or use `cast`?
This doesn't appear to be done.
================
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
----------------
chandlerc wrote:
> nit: Some vertical whitespace before this comment block would make it easier to read IMO.
nit: This is a blonk comment line rather than a blank line?
================
Comment at: lib/Transforms/Scalar/LowerExpectIntrinsic.cpp:144-145
+
+ bool IsPhiValueOne = ((!ValueInverted && !ExpectedValue->isZero()) ||
+ (ValueInverted && ExpectedValue->isZero()));
+
----------------
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.
================
Comment at: lib/Transforms/Scalar/LowerExpectIntrinsic.cpp:125
+ if (!BinOp || BinOp->getOpcode() != Instruction::Xor)
+ break;
+
----------------
My suggestion was somewhat specifically to use early return here rather than break?
================
Comment at: lib/Transforms/Scalar/LowerExpectIntrinsic.cpp:127
+
+ // Handle XOR:
+ Value *Opnd0 = BinOp->getOperand(0);
----------------
This comment isn't really helping. The code already is clearly handling XOR.
I would suggest something more like before you get the value as a binary operator saying:
// Lastly, we try to handle binary operators that either copy
// or invert the boolean value. These are canonicalized in LLVM
// to an `xor` instruction.
Once you have that, not sure you even need a comment here at all.
================
Comment at: lib/Transforms/Scalar/LowerExpectIntrinsic.cpp:128-129
+ // Handle XOR:
+ Value *Opnd0 = BinOp->getOperand(0);
+ Value *Opnd1 = BinOp->getOperand(1);
+ ConstantInt *CInt = dyn_cast<ConstantInt>(Opnd1);
----------------
These variables don't seem to help anything. They're each only used once. Sink the initializer to the one usage below?
================
Comment at: lib/Transforms/Scalar/LowerExpectIntrinsic.cpp:136-139
+ if (CInt->isMinusOne())
+ ValueInverted = !ValueInverted;
+ else
+ break;
----------------
This looks like it is wrong when the xor operand is neither zero nor -1 (possibly due to being on a zero-extended value). We move `V` to the operand which might be the phi, and then break out.
As I had suggested, I find this easier to read when structured using early return and it also avoids the bug:
if (!CInt->isZero()) {
if (!CInt->isMinusOne())
return;
ValueInverted = !ValueInverted;
}
That said, I would also personally only assign V to the operand after this block just to avoid confusion.
================
Comment at: lib/Transforms/Scalar/LowerExpectIntrinsic.cpp:154
+ BasicBlock *BB = PhiDef->getIncomingBlock(i);
+ BranchInst *BR = dyn_cast<BranchInst>(BB->getTerminator());
+ if (BR && BR->isConditional())
----------------
nit: `BI` is more commonly used for a branch instruction than `BR` in LLVM.
================
Comment at: lib/Transforms/Scalar/LowerExpectIntrinsic.cpp:166
+
+ // Now walk through all Phi operands:
+ for (unsigned i = 0, e = PhiDef->getNumIncomingValues(); i != e; ++i) {
----------------
An old comment that didn't get addressed but is on a different line now:
"""
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:175-177
+ // TODO: properly handlle non-i1 case. For instance, when the expected
+ // phi value is 10, and the phi operand value is 5, isUnlikely should
+ // be set to true.
----------------
We typically use 'FIXME'. I'd put this comment up on the computing of `IsPhiValueNonZero` as that's when this became confusing for me.
I also don't think the `IsUnlikely` variable is useful here. I would just sink it into the `if`.
The comment `Not an interesting case:` doesn't really tell the reader much. I think it would be more helpful to explain why only matched operands and values are meaningful to turn into metadata.
================
Comment at: lib/Transforms/Scalar/LowerExpectIntrinsic.cpp:187-192
+ // 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 of BR's successors:
----------------
Minor prose suggestions:
"Figuring out which" -> "Test whether a particular"
"that leads" -> "leads"
"triangular shaped" should either be just "triangular" or "triangle shaped". I mildly prefer the former.
Ending this with a ':' doesn't make much sense to me.
Overall, the second sentence here doesn't help me as much. I would maybe suggest putting the explanation of the "how does this work" attached to the code inside the lambda. I'll suggest things there.
================
Comment at: lib/Transforms/Scalar/LowerExpectIntrinsic.cpp:199
+ auto *Succ = BR->getSuccessor(succ);
+ if (OpndIncomingBB == Succ)
+ return true;
----------------
Maybe comment:
// If this successor is the incoming block for this Phi operand, then
// this successor does lead to the Phi.
However, I feel like this needs to check that `OpndIncomingBB->getUniquePredecessor()` returns non-null. Otherwise this might not be an actual diamond.
================
Comment at: lib/Transforms/Scalar/LowerExpectIntrinsic.cpp:201-202
+ return true;
+ if (OpndIncomingBB == BR->getParent() &&
+ Succ == PhiDef->getParent())
+ return true;
----------------
Maybe comment:
// Otherwise, if the edge is directly from the branch to the Phi,
// this successor is the one feeding this Phi operand.
================
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:
> 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...
================
Comment at: test/Transforms/LowerExpectIntrinsic/phi_merge.ll:12
+
+define void @foo(i32 %arg, i32 %arg1, i32 %arg2, i32 %arg3) #0 {
+bb:
----------------
`CHECK-LABEL`?
================
Comment at: test/Transforms/LowerExpectIntrinsic/phi_merge.ll:38
+ %tmp25 = icmp ne i64 %tmp24, 0
+; CHECK: br i1 %tmp25,{{.*}}!prof [[WEIGHT]]
+ br i1 %tmp25, label %bb26, label %bb28
----------------
I typically put checks after the IR they are matching against.
================
Comment at: test/Transforms/LowerExpectIntrinsic/phi_merge.ll:53
+
+define void @foo2(i32 %arg, i32 %arg1, i32 %arg2, i32 %arg3) #0 {
+bb:
----------------
`CHECK-LABEL`?
================
Comment at: test/Transforms/LowerExpectIntrinsic/phi_merge.ll:63
+ %tmp13 = icmp sgt i32 %arg1, %tmp12
+; CHECK-NOT: !prof
+ br i1 %tmp13, label %bb14, label %bb18
----------------
It would be good to make this a more precise check by first checking for the branch, and then checking that the profile data isn't there:
; CHECK: br i1 %tmp13,
; CHECK-NOT: !prof
https://reviews.llvm.org/D33164
More information about the llvm-commits
mailing list