[PATCH] D48105: [llvm][Instrumentation] Add Call Graph Profile pass

[David Li via Phabricator via llvm-commits]

davidxl added a comment.

I am with Chandler here. Adding this pass to the legacy PM only will slow down the eventual migration to the new PM. It is better to do the new PM or both.



================
Comment at: llvm/trunk/lib/Transforms/Instrumentation/CGProfile.cpp:52
+  for (auto &F : M) {
+    if (F.isDeclaration())
+      continue;
----------------
You can check the existence of function entry count to decide whether to skip function earlier.


================
Comment at: llvm/trunk/lib/Transforms/Instrumentation/CGProfile.cpp:61
+      for (const auto &I : BB) {
+        auto *CI = dyn_cast<CallInst>(&I);
+        if (!CI)
----------------
Use CallSite interface instead so that InvokeInst is handled as well:

   CallSite CS(&I);
    if (!CS) continue;

Chandler has plan to unify the CallInst and InvokeInst in the future, but for now, CallSIte is the way to go.


================
Comment at: llvm/trunk/lib/Transforms/Instrumentation/CGProfile.cpp:65
+        Function *CalledF = CI->getCalledFunction();
+        if (!CalledF || CalledF->isIntrinsic())
+          continue;
----------------
you may want to check TTI to see if isLoweredToCall returns true.


================
Comment at: llvm/trunk/lib/Transforms/Instrumentation/CGProfile.cpp:68
+
+        uint64_t &Count = Counts[std::make_pair(&F, CalledF)];
+        Count = SaturatingAdd(Count, *BBCount);
----------------
You may also check indirect callsite to see if it has value profile data (indirect call target). If it has, you can create edges to the targets as well.

(I assume this information is for function layout purpose by the linker).


Repository:
  rL LLVM

https://reviews.llvm.org/D48105