[PATCH] D40728: [CallSiteSplitting] Refactor creating callsites.

[Florian Hahn via Phabricator via llvm-commits]

fhahn added inline comments.


================
Comment at: lib/Transforms/Scalar/CallSiteSplitting.cpp:143
+  BasicBlock *To = Pred;
+  while ((From = From->getSinglePredecessor())) {
+    recordCondition(CS, From, To, Conditions);
----------------
junbuml wrote:
> We should not revisit the same predecessor. Otherwise, it might get into infinite loop if  header is unreachable from the entry and we have a backedge from TBB to header : 
> ```
> define i32 @test(i32* %a, i32 %v, i32 %p) {
> Entry:
>   br label %End
> Header:
>   %tobool2 = icmp eq i32 %p, 10
>   br i1 %tobool2, label %Tail, label %TBB
> TBB:
>   %cmp = icmp eq i32 %v, 1
>   br i1 %cmp, label %Tail, label %Header
> Tail:
>   %r = call i32 @callee(i32* %a, i32 %v, i32 %p)
>   ret i32 %r
> End:
>   ret i32 %v
> }
> ```
Indeed, should be fixed now. I've added this example to the tests.


================
Comment at: test/Transforms/CallSiteSplitting/callsite-split-or-phi.ll:354
+;CHECK: ret i32 %[[MERGED]]
+define i32 @test_eq_eq_eq_untaken(i32* %a, i32 %v, i32 %p) {
+Header:
----------------
fhahn wrote:
> junbuml wrote:
> > In order to prevent any future change from breaking this pass, I believe we need more test cases including eq_eq_eq_taken. It will be also good to add a test case where the same argument is used multiple ICMPs. 
> Agreed, I'll add such a test cases tomorrow.
I've added more test cases as suggested. I've also added tests where there are multiple constraints along a path (*constrain_same*). If there are conflicting conditions, e.g. x == 111 and x == 222, the first one seen will be used. I think that should be fine, as in that case the block should be unreachable. I've also mentioned that in the comment for `recordConditions`


https://reviews.llvm.org/D40728