[PATCH] D41860: [CallSiteSplitting] Support splitting of blocks with instrs before call.

[Jun Bum Lim via Phabricator via llvm-commits]

junbuml added a comment.

Overall LGTM, but I want someone else also look at this.



================
Comment at: lib/Transforms/Scalar/CallSiteSplitting.cpp:290
+    DenseMap<const Value *, Value *> Mapping;
+    for (const auto &KV : Remap)
+      Mapping[KV.first] = KV.second;
----------------
Isn't it possible to hold space for ValueToValueMapTy for each predecessor and pass its reference to DuplicateInstructionsInSplitBetween. 

For example, as we only support 2 predecessors for now, we can define Remaps as a fixed size array which has  ValueToValueMapTy as its element, and then we can pass the reference of each array element to DuplicateInstructionsInSplitBetween for each predecessor. 


================
Comment at: lib/Transforms/Scalar/CallSiteSplitting.cpp:313
+    if (!CurrentI->use_empty()) {
+      PHINode *NewPN = PHINode::Create(CurrentI->getType(), 2);
+      for (auto &Mapping : Remaps)
----------------
Instead of '2', it will be good to use the number of pred of TailBB. Or at least we need assert before using '2' directly here.


================
Comment at: test/Transforms/CallSiteSplitting/callsite-instructions-before-call.ll:158
+; CHECK: %[[V1:.+]] = load i32, i32*
+; CHECK: call void @bar(i32* null, i32 %0)
+; CHECK: br label %CallSite
----------------
I think it should be %i, instead of %0. Or did you intend [[V1]] ? 


https://reviews.llvm.org/D41860