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

[David Li via Phabricator via llvm-commits]

davidxl marked 8 inline comments as done.
davidxl added inline comments.


================
Comment at: lib/Transforms/Scalar/LowerExpectIntrinsic.cpp:96
+  ConstantInt *ExpectedValue = cast<ConstantInt>(Expect->getArgOperand(1));
+  uint64_t ExpectedPhiValue = ExpectedValue->getZExtValue();
+
----------------
chandlerc wrote:
> 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?)
Rewrote the code to use APInt. Also do a forward computation from the operand value using the recorded operations and compare with the expected value.   This is more flexible which allows us to handle other operations in the future. 


================
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)
----------------
chandlerc wrote:
> 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.) 
Added two sext test case.

>From i8 255 --> sext  to i64  and compare with 255   -- does not match -- unlikely annotation emitted.
>From i8 255 --> sext  to i64 and compare with -1  --> match -- no annotation.


https://reviews.llvm.org/D33164