[PATCH] D148876: [IndirectCallPromotion][SimpifyCFG] Preserve indirect callsites in hoist/sink when merging !prof is unprofitable and merge these callsites after ICP completes in a build pipeline

[Mingming Liu via Phabricator via llvm-commits]

mingmingl marked 4 inline comments as done.
mingmingl added inline comments.


================
Comment at: llvm/include/llvm/IR/Instructions.h:4018
+  static bool getProfMetaData(MDNode *MD, MDNode *&BranchWeightMD,
+                              MDNode *&IndirectCallMD) {
+    BranchWeightMD = IndirectCallMD = nullptr;
----------------
tejohnson wrote:
> Nothing in this patch uses the IndirectCallMD value set here - is it needed?
In the previous version this function was not called so should be deleted back then. Only `getBranchWeightProfData` was used. Now the change becomes obsolete..


================
Comment at: llvm/lib/Transforms/Instrumentation/IndirectCallPromotion.cpp:309
+  if (!MDProf || isa<CallInst>(CB)) {
+    CB->setMetadata(LLVMContext::MD_prof, nullptr);
+    return;
----------------
tejohnson wrote:
> This is unnecessary if !MDProf
Done in the sense that the code is obsolete


================
Comment at: llvm/lib/Transforms/Instrumentation/IndirectCallPromotion.cpp:313
+
+  if (!isa<InvokeInst>(CB))
+    return;
----------------
tejohnson wrote:
> Can the CallBase be anything other than a CallInst or InvokeInst?
Good question. Only CallInst or InvokeInst should be relevant for function calls, since CallBrInst is [[ I see many `isa<CallInst>(I) || isa<Invokeinst>` in the codebase.  | only used for inline assembly goto ]].

Added comment around helper function `getFunctionCallInstr` in `llvm/lib/IR/Metadata.cpp` for this.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D148876/new/

https://reviews.llvm.org/D148876