[PATCH] D40037: [CallSiteSplitting] Remove some indirection (NFC).

[Jun Bum Lim via Phabricator via llvm-commits]

junbuml added inline comments.


================
Comment at: lib/Transforms/Scalar/CallSiteSplitting.cpp:374
+  // contains the first compare of the OR and OrBB the second.
+  if (Preds[1]->getSinglePredecessor() == Preds[0]) {
+    HeaderBB = Preds[0];
----------------
fhahn wrote:
> junbuml wrote:
> > fhahn wrote:
> > > junbuml wrote:
> > > > In case HeaderBB has more than two successors (e.g, if a switch is it's terminator), this change could allow splitting.  If you want to keep this change NFC, the number of successors of HeaderBB may need to be checked. 
> > > I think for the OR case the checks for ICMP would catch this case later (as switch for i1 only can have 2 successors I think) , but I could add an explicit check.
> > Currently, the below test case is not handled, but with this change, we can split the call-site.  Although it's not the OR structure, it doesn't seem to break the original intention of this pass.  Looks like we can even extend this pass to support switch better.  However, if we want to support this in this change, I think we should run tests. 
> > 
> > ```
> > 
> > define i32 @test(i32* %a, i32 %i, i32 %v) {
> > Header:
> >   switch i32 %i, label %End [
> >     i32 0, label %BB0
> >     i32 1, label %BB1
> >     i32 2, label %BB2
> >     i32 3, label %Tail
> >     i32 4, label %Tail
> >   ]
> > 
> > BB0:
> >   br label %End
> > 
> > BB1:
> >   br label %End
> > 
> > BB2:
> >   %cmp = icmp eq i32 %v, 1
> >   br i1 %cmp, label %Tail, label %End
> > 
> > Tail:
> >   %r = call i32 @callee(i32* %a, i32 %v)
> >   ret i32 %r
> > 
> > End:
> >   ret i32 %v
> > }
> > 
> > ```
> I think it will be best to keep this change as a NFC and go from there, what do you think?
> 
> I would plan to update the patch to only do the transform if the header has 2 successors in the OR case and post a follow-up patch to enable the additional transform after that.
Sounds good to me. 


https://reviews.llvm.org/D40037