[PATCH] D33164: [Profile[ Enhance expect lowering to handle correlated branches
Chandler Carruth via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Thu Jun 1 14:29:51 PDT 2017
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
More information about the llvm-commits
mailing list