[PATCH] D38094: Separate the logic when handling indirect calls in SamplePGO ThinLTO compile phase and other phases.

[Teresa Johnson via Phabricator via llvm-commits]

tejohnson added a comment.

Is this patch NFC? It looks like there is some new handling for indirect call targets in findImportedFunctions that makes it non-NFC. Could you split this into an NFC refactoring patch and a separate non-NFC patch with the enhancements if so? I think that would be clearer and easier to review.



================
Comment at: include/llvm/ProfileData/SampleProf.h:342
       S.insert(Function::getGUID(Name));
+    for (const auto &BS : BodySamples)
+      for (const auto &TS : BS.second.getCallTargets())
----------------
Needs comment.


================
Comment at: include/llvm/Transforms/SampleProfile.h:29
   std::string ProfileFileName;
+  bool InThinLTOCompile;
 };
----------------
How about IsThinLTOPreLink for consistency with Phase? (Here and in other classes)


================
Comment at: lib/Transforms/IPO/SampleProfile.cpp:89
              "collected in the parent function, will be inlined again."));
+static cl::opt<bool> SampleProfileInThinLTOCompile(
+    "sample-profile-in-thinlto-compile", cl::init(false), cl::value_desc("N"),
----------------
Similar, let's use PreLink instead of Compile, to be consistent with terminology introduced for pass pipeline. After looking at how this is used, it does something different than what I originally thought. I first read this as whether to invoke this pass during the ThinLTO compile/pre-link, but it is actually forcing simulation of being in that phase. So maybe better to have something like "SampleProfileForceThinLTOPreLink" or similar.

I see this is used when invoked via opt. Can the phase be forced instead by using "-passes='thinlto-pre-link<O2>" (see test/Other/new-pm-thinlto-defaults.ll)?


https://reviews.llvm.org/D38094