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

[Matthew Simpson via Phabricator via llvm-commits]

mssimpso added a comment.

Thanks again for the comments, David. I'll update the patch shortly.



================
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:
> 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.
Sure, I'm fine restoring the interface if you'd prefer. The client can always swap the pointers after the call if needed.


================
Comment at: lib/Transforms/Utils/CallPromotionUtils.cpp:166
+///     %t1 = bitcast i32 %t0 to ...
+///     br label %normal_dst
+///
----------------
davidxl wrote:
> Add a test case for the splitBB  case for bitcast?
This case is reflected in the change to test/Transforms/PGOProfile/icp_covariant_invoke_return.ll. After versioning the invoke, the `if.true.direct_targ.if.end.icp_crit_edge` block, which contains the bitcast, is added between the invoke's parent block (the direct call bock), and it's normal destination (the merge block).


https://reviews.llvm.org/D40751