[PATCH] D40751: [ICP] Expose unconditional call promotion interface

[Matthew Simpson via Phabricator via llvm-commits]

mssimpso added inline comments.


================
Comment at: include/llvm/Transforms/Utils/CallPromotionUtils.h:47
+/// call were direct.
+void demoteCall(CallSite CS, Value *CalledValue, ArrayRef<CastInst *> Casts);
+
----------------
davidxl wrote:
> Can you remove this method in this patch? It is not used anywhere
Sure, that make sense. I'll add this part to D39869.


================
Comment at: lib/Transforms/IPO/SampleProfile.cpp:829
             uint64_t C = FS->getEntrySamples();
-            Instruction *DI =
-                pgo::promoteIndirectCall(I, R->getValue(), C, Sum, false, ORE);
+            Instruction *DI = I;
+            I = pgo::promoteIndirectCall(I, R->getValue(), C, Sum, false, ORE);
----------------
davidxl wrote:
> what is this change for?
The conditional promotion interface now modifies the given call site to be direct and returns the newly created indirect call site.  Previously the given call site was left unmodified, and a newly created direct call site was returned.  This change makes the conditional and unconditional promotion interfaces consistent with each other (the given call site is changed to be direct), and also works well with D39869.


================
Comment at: lib/Transforms/Instrumentation/IndirectCallPromotion.cpp:315
     Weights.push_back(Count);
-    MDBuilder MDB(NewInst->getContext());
-    NewInst->setMetadata(LLVMContext::MD_prof, MDB.createBranchWeights(Weights));
+    MDBuilder MDB(Inst->getContext());
+    Inst->setMetadata(LLVMContext::MD_prof, MDB.createBranchWeights(Weights));
----------------
davidxl wrote:
> What is this change for?
Same as above. "Inst" is now made to be direct, and "NewInst" is the new instruction in the "else" block.


================
Comment at: lib/Transforms/Utils/CallPromotionUtils.cpp:36
       continue;
-    Value *V = Phi->getIncomingValue(Idx);
-    if (dyn_cast<Instruction>(V) == Invoke) {
-      Phi->setIncomingBlock(Idx, ElseBlock);
-      Phi->addIncoming(NewInst, OrigBlock);
-      continue;
-    }
-    Phi->addIncoming(V, ElseBlock);
+    Phi->setIncomingBlock(Idx, MergeBlock);
   }
----------------
davidxl wrote:
> Can you explain this change (and other related interface changes)  perhaps with a tiny example -- there are lots of words in the description but not as helpful as an example.
Sure, I've added code snippets to all of the basic transformations with both call and invoke examples.

Basically, the code here is changing because the control-flow we create for conditional promotion is slightly different than it was previously. The reason it's different is described below. Previously, we generated code like:

```
orig_bb:
  %cond = icmp eq i32 ()* %ptr, @func
  br i1 %cond, %then_bb, %else_bb

then_bb:
  %t0 = invoke i32 @func() to label %merge_bb unwind label %unwind_dst

else_bb:
  %t1 = invoke i32 %ptr() to label %normal_dst unwind label %unwind_dst

merge_bb:
  br %normal_dst

normal_dst:
  %t2 = phi i32 [ %t0, %merge_bb ], [ %t1, %else_bb ]
```

Whereas now, we generate code like:

```
orig_bb:
  %cond = icmp eq i32 ()* %ptr, @func
  br i1 %cond, %then_bb, %else_bb

then_bb:
  %t0 = invoke i32 @func() to label %merge_bb unwind label %unwind_dst

else_bb:
  %t1 = invoke i32 %ptr() to label %merge_bb unwind label %unwind_dst

merge_bb:
  %t2 = phi i32 [ %t0, %then_bb ], [ %t1, %else_bb ]
  br %normal_dst
```

The difference is that now both invokes have "merge_bb" as their normal destination. Previously, only the normal destination of the promoted invoke was set to "merge_bb", where a return value bitcast was added if needed, and the phi node for merging the return values was created/modified in the original normal destination.  After this patch, the normal destination of both invokes is set to "merge_bb", where the merge phi node is added, and if a return value bitcast is required, the edge between "then_bb" and "merge_bb" is split and the bitcast is placed there.

Previously, there was no way to perform a stand-alone unconditional promotion because it was assumed that an if-then-else structure would always be created. We can do that now with this patch, and the refactoring allows greater code reuse between the conditional and unconditional cases. To perform conditional promotion, we just need to version the original invoke (which creates the if-then-else structure, adds the merge phi node, and fixes up phi nodes in the normal and unwind destinations), and then unconditionally promote one of them (which also adds the return value bitcast if necessary).  I hope that helps!


================
Comment at: lib/Transforms/Utils/CallPromotionUtils.cpp:279
+
+void llvm::demoteCall(CallSite CS, Value *CalledValue,
+                      ArrayRef<CastInst *> Casts) {
----------------
davidxl wrote:
> Remove this one from this patch.
This will be added to D39869


https://reviews.llvm.org/D40751