[PATCH] D17864: [PGO] Promote indirect calls to conditional direct calls with value-profile

[Rong Xu via llvm-commits]

xur marked 15 inline comments as done.

================
Comment at: lib/IR/Instructions.cpp:3016
@@ -3012,1 +3015,3 @@
+bool 
+CastInst::castIsValid(Instruction::CastOps op, Type *SrcTy, Type *DstTy) {
 
----------------
davidxl wrote:
> This change can be carved out as a NFC refactoring.
OK. Will split this code out to a separated patch.

================
Comment at: lib/Transforms/IPO/PassManagerBuilder.cpp:590
@@ +589,3 @@
+  // the earlier promotion pass that promotes intra-moduletargets.
+  // This two-step promotion is to save the compile time. For LTO, it should
+  // produce the same result as if we only do promotion here.
----------------
davidxl wrote:
> can you explain the 'save compile time' part here?
As I mentioned in earlier reply. the optimization performed in LTOOptimizationPasses cannot be palatalized. The Intra-module promotion can be trivially paralleled. LTO part of promotion takes pretty long for moderately large programs like 483.xalancbmk in spec2006. Splitting into two part saves the time for parallel builds.

================
Comment at: lib/Transforms/Instrumentation/IndirectCallPromotion.cpp:185
@@ +184,3 @@
+
+  uint32_t tryToPromote(Instruction *Inst,
+                        const std::vector<CandidatePromotion> &Candidates,
----------------
davidxl wrote:
> Document this method -- what is passed in and what is returned.
will do.

================
Comment at: lib/Transforms/Instrumentation/IndirectCallPromotion.cpp:223
@@ +222,3 @@
+    return false;
+  if (TotalCount < Count)
+    Count = TotalCount;
----------------
davidxl wrote:
> When can this happen? Should it be corrected in caller?
will have this checked in the caller.

================
Comment at: lib/Transforms/Instrumentation/IndirectCallPromotion.cpp:234
@@ +233,3 @@
+  StringRef TargetFuncName = Symtab->getFuncName(Target);
+  if (TargetFuncName.empty())
+    return NotAvailableInModule;
----------------
davidxl wrote:
> Is this check needed? NotAvailableInModule is essentially body not available.
decls might be available.
just for debug.
I'll merge this two cases.

================
Comment at: lib/Transforms/Instrumentation/IndirectCallPromotion.cpp:378
@@ +377,3 @@
+  // Add jump from Merge BB to the NormalDest.
+  BranchInst::Create(II->getNormalDest(), *MergeBB);
+}
----------------
davidxl wrote:
> Do you need merge BB here for invoke at all (which will be empty and unreachable)?
I assume you are referring this to the invoke instr.
I need this BB to insert a potential bit-cast inst.
If there is not bit-cast, it will be just a branch to the original destBB. 
I assume this gonna be clean by late CFG optimization. 



================
Comment at: lib/Transforms/Instrumentation/IndirectCallPromotion.cpp:439
@@ +438,3 @@
+    II->mutateFunctionType(DirectCallee->getFunctionType());
+    II->setNormalDest(NormalDest);
+  }
----------------
davidxl wrote:
> if you remove MergeBB, there is no need for this step -- or this is needed to avoid fixing up phis in the original normal dest?
as I mentioned early, this is for bit-cast, not for phis.

================
Comment at: lib/Transforms/Instrumentation/IndirectCallPromotion.cpp:555
@@ +554,3 @@
+                                       Inst);
+  Instruction *DirectCallResult =
+      insertCallRetCast(Inst, NewInst, DirectCallee);
----------------
davidxl wrote:
> Can this be folded into create4DirectCallInst call?
I remember there is a transformation ordering issue. but I can try. will comment this more in the new patch.


http://reviews.llvm.org/D17864