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

[Dehao Chen via Phabricator via llvm-commits]

danielcdh added inline comments.


================
Comment at: lib/Transforms/Utils/InlineFunction.cpp:1441
+  for (auto const &Entry : VMap)
+    if (isa<CallInst>(Entry.first) && &*Entry.second != nullptr &&
+        isa<CallInst>(Entry.second))
----------------
zzheng wrote:
> Isn't it redundant to check ptr != nullptr?
> 
> Is there any chance that Entry.second is NULL and what happen on (*Entry.second) in that case?
It indeed happened once when I compile large code, that's why I added this check. I'm not familiar with cloning code, Easwaran may have some insights on why this could be nullptr? Or it's a bug in function clone?


================
Comment at: lib/Transforms/Utils/InlineFunction.cpp:1450
+          // No need to update the callsite if it is pruned during inlining.
+          if (VMap.count(CI))
+            CI->updateProfWeight(CalleeEntryCount.getValue() - CallCount,
----------------
eraman wrote:
> The check above is not needed. After all, if the block is reachable in the call context, all instructions inside are reachable too. But there is one tricky case. Let's say that a call has been simplified into a value (this can happen during pruning), Should you update the call count or not? If you should not update, then the check above should be isa<CallInst>(VMap.Count(CI)). I think updating is the right thing here though.
The prof count only attaches to branch instructions. For the case you mentioned, it's already checked by the "if (CallInst *CI = dyn_cast<CallInst>(&I))", thus I simply removed the second check.


https://reviews.llvm.org/D30767