[PATCH] D30767: Updates branch_weights annotation for call instructions during inlining.

[Easwaran Raman via Phabricator via llvm-commits]

eraman requested changes to this revision.
eraman added inline comments.
This revision now requires changes to proceed.


================
Comment at: include/llvm/IR/Instruction.h:257
+  /// is true, update branch_weight with 1 - \p R.
+  void updateProfWeight(double R, bool Reverse);
+
----------------
Do not use double here and instead specify it as a fraction.


================
Comment at: lib/IR/Instruction.cpp:691
+    auto *V = mdconst::dyn_extract<ConstantInt>(ProfileData->getOperand(i));
+    if (!V)
+      return;
----------------
What are the assumptions on the MD_prof metadata here? Could you assert V here? If it is indeed possible that ith operand is not constant int, could the next operand be constant int (in which case you shouldn't return here)


================
Comment at: lib/Transforms/Utils/InlineFunction.cpp:1427
 }
 
+static void updateCallProfile(Function *Callee,
----------------
Document this method. Also,  perhaps choose a more meaningful name.


================
Comment at: lib/Transforms/Utils/InlineFunction.cpp:1442
+  for (auto const &Entry : VMap) {
+    if (!isa<BasicBlock>(Entry.first) || !Entry.second)
+      continue;
----------------
No need to keep track of the cloned blocks and update the calls inside them. VMap also includes the instructions - so just check if they are calls and update the weights.


================
Comment at: lib/Transforms/Utils/InlineFunction.cpp:1453
+      if (CallInst *CI = dyn_cast<CallInst>(&I))
+        CI->updateProfWeight(Ratio, true);
+}
----------------
There is a subtle issue here. Let's say there is a block in the callee that is not executed in this call context and that block has a call instruction. You'll still be reducing the weight of that call even though you haven't cloned it. Similar issue does exist in updating the bfi incrementally, but that's okay because BFI does get recomputed later. In this case, once the weight gets incorrectly updated it remains so forever. What you should be doing is update the weights only when the call has been cloned.


================
Comment at: lib/Transforms/Utils/InlineFunction.cpp:1471
+  if (!CallInst->extractProfTotalWeight(CallCount))
+    if (CallerBFI) {
+      Optional<uint64_t> CallSiteCount = CallerBFI->getBlockProfileCount(CallBB);
----------------
This code that gets either the metadata weight or the block weight should be refactored and moved out of here.


================
Comment at: lib/Transforms/Utils/InlineFunction.cpp:1483
   else
-    Callee->setEntryCount(CalleeCount.getValue() - CallSiteCount.getValue());
+    Callee->setEntryCount(CalleeCount.getValue() - CallCount);
 }
----------------
This gets confusing. You're now updating the entry count based on the metadata. When sample profile is used, is it expected that the sum of the calls' profile weights equal the entry count? Do you smooth out the entry count based on call instructions' profile weight  when you initially load the profile. In any case, more comments are helpful.


================
Comment at: test/Transforms/Inline/prof-update.ll:1
+; RUN: opt < %s -inline -S | FileCheck %s
+; Checks if inliner updates branch_weights annotation for call instructions.
----------------
Please expand this test or add a new test case to handle more complex cases where some calls are not cloned into the caller.


https://reviews.llvm.org/D30767