Update VP prof metadata during inlining.

[danielcdh]

Summary

r298270 added profile update logic for branch_weights. This patch implements profile update logic for VP prof metadata too.

[eraman]

lib/IR/Instruction.cpp

637

This is not easy to read. Why are only odd indices pushed into Vals? I suppose you want to scale only operand 2. I suggest you separate the two cases (VP aand branch_weights) and handle them separately.

[danielcdh]

update

[tejohnson]

I found that not all calls with VP metadata were being scaled after this patch. Specifically, this was VP metadata on a memcpy intrinsic (so profiling the memcpy size values not an indirect call), but it still makes sense to scale them the same way. I confirmed that the call was not in the value map iterated in the caller of this function (updateCallProfile), which is what is iterated to update the inlined instructions in the caller. Are we guaranteed that every inlined instruction would be in there?

[tejohnson]

In D32773#745551, @tejohnson wrote:

I found that not all calls with VP metadata were being scaled after this patch. Specifically, this was VP metadata on a memcpy intrinsic (so profiling the memcpy size values not an indirect call), but it still makes sense to scale them the same way. I confirmed that the call was not in the value map iterated in the caller of this function (updateCallProfile), which is what is iterated to update the inlined instructions in the caller. Are we guaranteed that every inlined instruction would be in there?

This may be a false alarm. It looks like we might simply not bed doing the update, which would happen if the entry count of the callee was 0. My guess is that this happened because I was only redoing the ThinLTO backend with the patch, and we probably needed to do scaling in the compile step to avoid importing and inlining a callee with 0 entry count and unscaled VP metadata...confirming with a full rebuild, which will take some time.

[tejohnson]

In D32773#746315, @tejohnson wrote:

In D32773#745551, @tejohnson wrote:

I found that not all calls with VP metadata were being scaled after this patch. Specifically, this was VP metadata on a memcpy intrinsic (so profiling the memcpy size values not an indirect call), but it still makes sense to scale them the same way. I confirmed that the call was not in the value map iterated in the caller of this function (updateCallProfile), which is what is iterated to update the inlined instructions in the caller. Are we guaranteed that every inlined instruction would be in there?

This may be a false alarm. It looks like we might simply not bed doing the update, which would happen if the entry count of the callee was 0. My guess is that this happened because I was only redoing the ThinLTO backend with the patch, and we probably needed to do scaling in the compile step to avoid importing and inlining a callee with 0 entry count and unscaled VP metadata...confirming with a full rebuild, which will take some time.

Confirmed that this was a false alarm. In the source file I was looking at the expected functions are marked cold. So LGTM, but please wait for Easwaran to see if he has any more concerns.

[eraman]

LGTM after hoisting the APInt outside the loop (see inlined comment).

lib/IR/Instruction.cpp

643

Creating APInt objects > 64 bits in length requires heap allocation. So the loop invariant APInt objects (S and T) should be hoisted out. I know this is pre-existing, but it is better to fix this since similar code exists for VP (and where the loop likely executes more iterations).

[danielcdh]

update

[eraman]

lib/IR/Instruction.cpp

635

APInt(128, T) should also be hoisted here.

[danielcdh]

update

[eraman]

LGTM

lib/IR/Instruction.cpp

 }
 
 void Instruction::updateProfWeight(uint64_t S, uint64_t T) {
   auto *ProfileData = getMetadata(LLVMContext::MD_prof);
   if (ProfileData == nullptr)
     return;
 
   auto *ProfDataName = dyn_cast<MDString>(ProfileData->getOperand(0));
-  if (!ProfDataName || !ProfDataName->getString().equals("branch_weights"))
+  if (!ProfDataName || (!ProfDataName->getString().equals("branch_weights") &&
+                        !ProfDataName->getString().equals("VP")))
     return;
 
-  SmallVector<uint32_t, 4> Weights;
-  for (unsigned i = 1; i < ProfileData->getNumOperands(); i++) {
-    // Using APInt::div may be expensive, but most cases should fit in 64 bits.
-    APInt Val(128, mdconst::dyn_extract<ConstantInt>(ProfileData->getOperand(i))
-                       ->getValue()
-                       .getZExtValue());
+  MDBuilder MDB(getContext());
+  SmallVector<Metadata *, 3> Vals;
+  Vals.push_back(ProfileData->getOperand(0));
+
+  for (unsigned i = 1; i < ProfileData->getNumOperands(); i++)
+    if ((i % 2) == 1 && ProfDataName->getString().equals("VP")) {
+      Vals.push_back(ProfileData->getOperand(i));
+    } else {
+      // Using APInt::div may be expensive, but most cases should fit 64 bits.
+      APInt Val(128,
+                mdconst::dyn_extract<ConstantInt>(ProfileData->getOperand(i))
       Val *= APInt(128, S);
-    Weights.push_back(Val.udiv(APInt(128, T)).getLimitedValue());
+      Vals.push_back(MDB.createConstant(
+          ConstantInt::get(Type::getInt64Ty(getContext()),
+                           Val.udiv(APInt(128, T)).getLimitedValue())));
     }
-  MDBuilder MDB(getContext());
-  setMetadata(LLVMContext::MD_prof, MDB.createBranchWeights(Weights));
+  setMetadata(LLVMContext::MD_prof, MDNode::get(getContext(), Vals));
 }
 
 void Instruction::setProfWeight(uint64_t W) {
   assert((isa<CallInst>(this) || isa<InvokeInst>(this)) &&
          "Can only set weights for call and invoke instrucitons");
   SmallVector<uint32_t, 1> Weights;
   Weights.push_back(W);
   MDBuilder MDB(getContext());
   setMetadata(LLVMContext::MD_prof, MDB.createBranchWeights(Weights));
 }

test/Transforms/Inline/prof-update.ll

 ; RUN: opt < %s -inline -S | FileCheck %s
 ; Checks if inliner updates branch_weights annotation for call instructions.
 
 declare void @ext();
 declare void @ext1();
+@func = global void ()* null
 
 ; CHECK: define void @callee(i32 %n) !prof ![[ENTRY_COUNT:[0-9]*]]
 define void  @callee(i32 %n) !prof !1 {
   %cond = icmp sle i32 %n, 10
   br i1 %cond, label %cond_true, label %cond_false
 cond_true:
 ; ext1 is optimized away, thus not updated.
 ; CHECK: call void @ext1(), !prof ![[COUNT_CALLEE1:[0-9]*]]
   call void @ext1(), !prof !2
   ret void
 cond_false:
 ; ext is cloned and updated.
 ; CHECK: call void @ext(), !prof ![[COUNT_CALLEE:[0-9]*]]
   call void @ext(), !prof !2
+  %f = load void ()*, void ()** @func
+; CHECK: call void %f(), !prof ![[COUNT_IND_CALLEE:[0-9]*]] 
+  call void %f(), !prof !4
   ret void
 }
 
 ; CHECK: define void @caller()
 define void @caller() {
 ; CHECK: call void @ext(), !prof ![[COUNT_CALLER:[0-9]*]]
+; CHECK: call void %f.i(), !prof ![[COUNT_IND_CALLER:[0-9]*]]
   call void @callee(i32 15), !prof !3
   ret void
 }
 
 !llvm.module.flags = !{!0}
 !0 = !{i32 1, !"MaxFunctionCount", i32 2000}
 !1 = !{!"function_entry_count", i64 1000}
 !2 = !{!"branch_weights", i64 2000}
 !3 = !{!"branch_weights", i64 400}
+!4 = !{!"VP", i32 0, i64 140, i64 111, i64 80, i64 222, i64 40, i64 333, i64 20}
 attributes #0 = { alwaysinline }
 ; CHECK: ![[ENTRY_COUNT]] = !{!"function_entry_count", i64 600}
 ; CHECK: ![[COUNT_CALLEE1]] = !{!"branch_weights", i64 2000}
-; CHECK: ![[COUNT_CALLEE]] = !{!"branch_weights", i32 1200}
-; CHECK: ![[COUNT_CALLER]] = !{!"branch_weights", i32 800}
+; CHECK: ![[COUNT_CALLEE]] = !{!"branch_weights", i64 1200}
+; CHECK: ![[COUNT_IND_CALLEE]] = !{!"VP", i32 0, i64 84, i64 111, i64 48, i64 222, i64 24, i64 333, i64 12}
+; CHECK: ![[COUNT_CALLER]] = !{!"branch_weights", i64 800}
+; CHECK: ![[COUNT_IND_CALLER]] = !{!"VP", i32 0, i64 56, i64 111, i64 32, i64 222, i64 16, i64 333, i64 8}