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

[David Li via Phabricator via llvm-commits]

davidxl added inline comments.


================
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);
----------------
mssimpso wrote:
> 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.
I don't see  pgo::promoteIndirectCall interface documentation  change in this patch (in the header).

Besides, I am not sure the proposed interface change is better -- it is more natural to return the promoted 'direct call' instruction and modify I (reference) to be the indirect call, instead of requiring the client to do DI = I before the call.


================
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));
----------------
mssimpso wrote:
> 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.
It is very confusing -- this is the reason I don't like the interface change. A reader will naturally think NewInst is the newly created direct call (the implementation may still choose to modify the existing instruction and return it). 


================
Comment at: lib/Transforms/Utils/CallPromotionUtils.cpp:166
+///     %t1 = bitcast i32 %t0 to ...
+///     br label %normal_dst
+///
----------------
Add a test case for the splitBB  case for bitcast?


https://reviews.llvm.org/D40751