Directly return promoted direct call instead of rely on stripPointerCast.

[danielcdh]

Summary

stripPointerCast is not reliably returning the value that's being type-casted. Instead it may look further at function attributes to further propagate the value. Instead of relying on stripPOintercast, the more reliable solution is to directly use the pointer to the promoted direct call.

[tejohnson]

LGTM although I have a question on test

lib/Transforms/Instrumentation/IndirectCallPromotion.cpp

565

Suggest adding a comment above here about what CastInst vs NewInst are for clarity.

test/Transforms/SampleProfile/indirect-call.ll

100

How is it checking the returned parameter?

[danielcdh]

Thanks.

test/Transforms/SampleProfile/indirect-call.ll

100

Updated the comment to make it clearer. Please let me know if that clarifies the question.

[danielcdh]

update comments

[tejohnson]

test/Transforms/SampleProfile/indirect-call.ll

100

Ok thanks.

lib/Transforms/IPO/SampleProfile.cpp

for (auto I : CIS) {
           if (R != SymbolMap.end() && R->getValue() &&
               !R->getValue()->isDeclaration() &&
               R->getValue()->getSubprogram() &&
               isLegalToPromote(I, R->getValue(), &Reason)) {
             // The indirect target was promoted and inlined in the profile,
             // as a result, we do not have profile info for the branch
             // probability. We set the probability to 80% taken to indicate
             // that the static call is likely taken.
-            Instruction *DI = dyn_cast<Instruction>(
-                promoteIndirectCall(I, R->getValue(), 80, 100, false, ORE)
-                    ->stripPointerCasts());
+            Instruction *DI = promoteIndirectCall(
+                I, R->getValue(), 80, 100, false, ORE);
             PromotedInsns.insert(I);
             // If profile mismatches, we should not attempt to inline DI.
             if ((isa<CallInst>(DI) || isa<InvokeInst>(DI)) &&
                 inlineCallInstruction(DI))
               LocalChanged = true;
           } else {
             DEBUG(dbgs()
                   << "\nFailed to promote indirect call to "

lib/Transforms/Instrumentation/IndirectCallPromotion.cpp

 
 // Create a DirectCall instruction in the DirectCallBB.
 // Parameter Inst is the indirect-call (invoke) instruction.
 // DirectCallee is the decl of the direct-call (invoke) target.
 // DirecallBB is the BB that the direct-call (invoke) instruction is inserted.
 // MergeBB is the bottom BB of the if-then-else-diamond after the
 // transformation. For invoke instruction, the edges from DirectCallBB and
 // IndirectCallBB to MergeBB are removed before this call (during
-// createIfThenElse).
+// createIfThenElse). Stores the pointer to the Instruction that cast
+// the direct call in \p CastInst.
 static Instruction *createDirectCallInst(const Instruction *Inst,
                                          Function *DirectCallee,
                                          BasicBlock *DirectCallBB,
-                                         BasicBlock *MergeBB) {
+                                         BasicBlock *MergeBB,
+                                         Instruction *&CastInst) {
   Instruction *NewInst = Inst->clone();
   if (CallInst *CI = dyn_cast<CallInst>(NewInst)) {
     CI->setCalledFunction(DirectCallee);
     CI->mutateFunctionType(DirectCallee->getFunctionType());
   } else {
     // Must be an invoke instruction. Direct invoke's normal destination is
     // fixed up to MergeBB. MergeBB is the place where return cast is inserted.
     // Also since IndirectCallBB does not have an edge to MergeBB, there is no
for (unsigned I = 0; I < ParamNum; ++I) {
     Type *ATy = NewCS.getArgument(I)->getType();
     Type *PTy = DirectCalleeType->getParamType(I);
     if (ATy != PTy) {
       BitCastInst *BI = new BitCastInst(NewCS.getArgument(I), PTy, "", NewInst);
       NewCS.setArgument(I, BI);
     }
   }
 
-  return insertCallRetCast(Inst, NewInst, DirectCallee);
+  CastInst = insertCallRetCast(Inst, NewInst, DirectCallee);
+  return NewInst;
 }
 
 // Create a PHI to unify the return values of calls.
 static void insertCallRetPHI(Instruction *Inst, Instruction *CallResult,
                              Function *DirectCallee) {
   if (Inst->getType()->isVoidTy())
     return;
 
Instruction *llvm::promoteIndirectCall(Instruction *Inst,
   (void)BB;
   DEBUG(dbgs() << "\n\n== Basic Block Before ==\n");
   DEBUG(dbgs() << *BB << "\n");
 
   BasicBlock *DirectCallBB, *IndirectCallBB, *MergeBB;
   createIfThenElse(Inst, DirectCallee, Count, TotalCount, &DirectCallBB,
                    &IndirectCallBB, &MergeBB);
 
+  // If the return type of the NewInst is not the same as the Inst, a CastInst
+  // is needed for type casting. Otherwise CastInst is the same as NewInst.
+  Instruction *CastInst = nullptr;
   Instruction *NewInst =
-      createDirectCallInst(Inst, DirectCallee, DirectCallBB, MergeBB);
+      createDirectCallInst(Inst, DirectCallee, DirectCallBB, MergeBB, CastInst);
 
   if (AttachProfToDirectCall) {
     SmallVector<uint32_t, 1> Weights;
     Weights.push_back(Count);
     MDBuilder MDB(NewInst->getContext());
-    if (Instruction *DI = dyn_cast<Instruction>(NewInst->stripPointerCasts()))
-      DI->setMetadata(LLVMContext::MD_prof, MDB.createBranchWeights(Weights));
+    NewInst->setMetadata(LLVMContext::MD_prof, MDB.createBranchWeights(Weights));
   }
 
   // Move Inst from MergeBB to IndirectCallBB.
   Inst->removeFromParent();
   IndirectCallBB->getInstList().insert(IndirectCallBB->getFirstInsertionPt(),
                                        Inst);
 
   if (InvokeInst *II = dyn_cast<InvokeInst>(Inst)) {
     // At this point, the original indirect invoke instruction has the original
     // UnwindDest and NormalDest. For the direct invoke instruction, the
     // NormalDest points to MergeBB, and MergeBB jumps to the original
     // NormalDest. MergeBB might have a new bitcast instruction for the return
     // value. The PHIs are with the original NormalDest. Since we now have two
     // incoming edges to NormalDest and UnwindDest, we have to do some fixups.
     //
     // UnwindDest will not use the return value. So pass nullptr here.
     fixupPHINodeForUnwind(Inst, II->getUnwindDest(), MergeBB, IndirectCallBB,
                           DirectCallBB);
     // We don't need to update the operand from NormalDest for DirectCallBB.
     // Pass nullptr here.
     fixupPHINodeForNormalDest(Inst, II->getNormalDest(), MergeBB,
-                              IndirectCallBB, NewInst);
+                              IndirectCallBB, CastInst);
   }
 
-  insertCallRetPHI(Inst, NewInst, DirectCallee);
+  insertCallRetPHI(Inst, CastInst, DirectCallee);
 
   DEBUG(dbgs() << "\n== Basic Blocks After ==\n");
   DEBUG(dbgs() << *BB << *DirectCallBB << *IndirectCallBB << *MergeBB << "\n");
 
   using namespace ore;
   if (ORE)
     ORE->emit(OptimizationRemark(DEBUG_TYPE, "Promoted", Inst)
               << "Promote indirect call to " << NV("DirectCallee", DirectCallee)

test/Transforms/SampleProfile/Inputs/indirect-call.prof

  1: foo_inline_strip_conflict:3000
   1: 3000
 test_norecursive_inline:3000:0
  1: test_norecursive_inline:3000
   20: 3000
 test_noinline_bitcast:3000:0
  1: foo_direct_i32:3000
   1: 3000
+return_arg_caller:3000:0
+ 1: foo_inline1:3000
+  11: 3000
+ 2: return_arg:3000
+  1: 3000

test/Transforms/SampleProfile/indirect-call.ll

 define void @test_norecursive_inline() !dbg !24 {
 ; CHECK-NOT: icmp
 ; CHECK: call
   %1 = load void ()*, void ()** @y, align 8
   call void %1(), !dbg !25
   ret void
 }
 
+define i32* @return_arg(i32* readnone returned) !dbg !29{
+  ret i32* %0
+}
+
+; CHECK-LABEL: @return_arg_caller
+; When the promoted indirect call returns a parameter that was defined by the
+; return value of a previous direct call. Checks both direct call and promoted
+; indirect call are inlined.
+define i32* @return_arg_caller(i32* (i32*)* nocapture) !dbg !30{
+; CHECK-NOT: call i32* @foo_inline1
+; CHECK: if.true.direct_targ:
+; CHECK-NOT: call
+; CHECK: if.false.orig_indirect:
+; CHECK: call
+  %2 = call i32* @foo_inline1(i32* null), !dbg !31
+  %cmp = icmp ne i32* %2, null
+  br i1 %cmp, label %then, label %else
+
+then:
+  %3 = tail call i32* %0(i32* %2), !dbg !32
+  ret i32* %3
+
+else:
+  ret i32* null
+}
+
 @x = global i32 0, align 4
 @y = global void ()* null, align 8
 
 define i32* @foo_inline1(i32* %x) !dbg !14 {
   ret i32* %x
 }
 
 define i32* @foo_inline_strip.suffix(i32* %x) !dbg !15 {

 !21 = distinct !DISubprogram(name: "foo_direct", scope: !1, file: !1, line: 21, unit: !0)
 !22 = distinct !DISubprogram(name: "test_direct", scope: !1, file: !1, line: 22, unit: !0)
 !23 = !DILocation(line: 23, scope: !22)
 !24 = distinct !DISubprogram(name: "test_norecursive_inline", scope: !1, file: !1, line: 12, unit: !0)
 !25 = !DILocation(line: 13, scope: !24)
 !26 = distinct !DISubprogram(name: "test_noinline_bitcast", scope: !1, file: !1, line: 12, unit: !0)
 !27 = !DILocation(line: 13, scope: !26)
 !28 = distinct !DISubprogram(name: "foo_direct_i32", scope: !1, file: !1, line: 11, unit: !0)
+!29 = distinct !DISubprogram(name: "return_arg", scope: !1, file: !1, line: 11, unit: !0)
+!30 = distinct !DISubprogram(name: "return_arg_caller", scope: !1, file: !1, line: 11, unit: !0)
+!31 = !DILocation(line: 12, scope: !30)
+!32 = !DILocation(line: 13, scope: !30)