[PATCH] D41858: [CallSiteSplitting] Pass list of (BB, Conditions) pairs to splitCallSite.

[Jun Bum Lim via Phabricator via llvm-commits]

junbuml accepted this revision.
junbuml added a comment.
This revision is now accepted and ready to land.

LGTM !



================
Comment at: lib/Transforms/Scalar/CallSiteSplitting.cpp:112
+typedef std::pair<ICmpInst *, unsigned> ConditionTy;
+typedef SmallVector<ConditionTy, 3> ConditionsTy;
+
----------------
fhahn wrote:
> junbuml wrote:
> > Why don't we use 2 instead of 3? 
> We potentially record more than 2 conditions now I think and I thought 3 might be better suited to avoid dynamic allocations. I am happy to change it to 2 if you prefer that.
I'm okay with 3, but I don't think this change increase  the possibility of recording more than 2 conditions through single predecessors.  


================
Comment at: lib/Transforms/Scalar/CallSiteSplitting.cpp:152
+static void addConditions(CallSite CS,
+                          const SmallVectorImpl<ConditionTy> &Conditions) {
   for (auto &Cond : Conditions) {
----------------
fhahn wrote:
> junbuml wrote:
> > Why not  use  ConditionsTy  here ? 
> This function does not require knowing the size of the SmallVector, so I thought there is no need to make the type more restrictive. I am happy to change it if you prefer that
As it's used only with ConditionsTy tightly in our current implementation, I may prefer to be consistent with ConditionsTy for now. 


================
Comment at: lib/Transforms/Scalar/CallSiteSplitting.cpp:282-283
+
+    for (auto P : Splits)
+      CallPN->addIncoming(P.CallInst, P.BB);
+    Instr->replaceAllUsesWith(CallPN);
----------------
junbuml wrote:
> It appears that this loop can be merged into the above  loop (line 251), and then we can remove SplitInfo.
Thanks for handling this. 


https://reviews.llvm.org/D41858