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

[Easwaran Raman via Phabricator via llvm-commits]

eraman added inline comments.


================
Comment at: lib/IR/Instruction.cpp:693
+                       .getZExtValue();
+    Weights.push_back(Val * S / T);
+  }
----------------
For counts from instrumentation based profile, I use a 128 bit APInt to do the arithmetic and then educe it to 64 bit. In this case, since the values being multiplied are sample profile weights, I suppose the likelihood of Val * S overflowing 64 bits is very small and the APInt may be unnecessary. Even then, use saturated multiply so that the values are at least sensible even in the unlikely case of overflow.


================
Comment at: lib/Transforms/Utils/InlineFunction.cpp:1441
+  for (auto const &Entry : VMap)
+    if (isa<CallInst>(Entry.first))
+      cast<CallInst>(Entry.second)
----------------
You have to check if Entry.second is a call. It is possible for call instructions to be simplified to a value during the cloning. 


================
Comment at: lib/Transforms/Utils/InlineFunction.cpp:1447
+      if (CallInst *CI = dyn_cast<CallInst>(&I))
+        if (VMap.count(CI))
+          CI->updateProfWeight(CalleeEntryCount.getValue() - CallCount,
----------------
Useful to add a comment as to why this is done only when the instruction is in VMap.


================
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.
----------------
Nit: I prefer writing comments to explain why I expect the values in the CHECK statements. This helps anyone trying to modify the test later. 


https://reviews.llvm.org/D30767