Pass sample pgo flags to thinlto.

[danielcdh]

Summary

ThinLTO needs to invoke SampleProfileLoader pass during link time in order to annotate profile correctly after module importing.

[davide]

Do we need a similar change for lld?

[pcc]

Do we need to add the sample profile to the hash in computeCacheKey?

[mehdi_amini]

In D27790#623176, @pcc wrote:

Do we need to add the sample profile to the hash in computeCacheKey?

I'm concerned about this indeed.

Why is this SampleProfileLoaderPass not performed during the compile phase?

[danielcdh]

In D27790#623195, @mehdi_amini wrote:

In D27790#623176, @pcc wrote:

Do we need to add the sample profile to the hash in computeCacheKey?

I was planning to have it in a separate patch as it needs to hash the profile content too. Let me know if you think it should be part of this patch.

I'm concerned about this indeed.

Why is this SampleProfileLoaderPass not performed during the compile phase?

It is invoked in compiler phase. But if the profile is collected from ThinLTO binary, i.e. there is cross-module inlines in the profiling binary (thus in profile), the profile annotation needs to happen after all these inlines happened. i.e. annotation needs to be invoked once again in the linking phase.

Thanks,
Dehao

[mehdi_amini]

Ouch, I see: with sample-based profiling you can't change the callgraph after the instrumentation point to have the correct information, correct? Is changing the CFG OK though? (If you have a pointer about sample-based profiling where I can find the answer to these subtle points).

Hashing the full content of the profiles would break the "fine" grain of incremental build we have now. So I'd be cautious about that, even if this is likely a marginal use-case.

[danielcdh]

Considering sample pgo profile as a weighted-forest of inline stacks, each root corresponds to a symbol in the profile binary. The job of profile preparation is to expand the IR to resemble the hot part of the forest as in profile. More details about the sample pgo can be found in http://dl.acm.org/citation.cfm?id=2854044

At ThinLTO compile step, not all nodes is expandable as they could come from other modules. That's why we need to have SampleProfileLoader invoked after all nodes are visible.

In Sample PGO, the change of profile is far less frequent than source code. But once the profile is changed, the incremental build will be broken: everything needs to be built from scratch. I guess the same applies to instrumentation based pgo?

[mehdi_amini]

In D27790#623209, @danielcdh wrote:

Considering sample pgo profile as a weighted-forest of inline stacks, each root corresponds to a symbol in the profile binary. The job of profile preparation is to expand the IR to resemble the hot part of the forest as in profile. More details about the sample pgo can be found in http://dl.acm.org/citation.cfm?id=2854044

At ThinLTO compile step, not all nodes is expandable as they could come from other modules. That's why we need to have SampleProfileLoader invoked after all nodes are visible.

I have to read the citation to understand :)

In Sample PGO, the change of profile is far less frequent than source code. But once the profile is changed, the incremental build will be broken: everything needs to be built from scratch. I guess the same applies to instrumentation based pgo?

I don't think so: for instrumentation based PGO is applied during the compile phase (there is certainly a key difference that does not make it possible with the sample based one).

[danielcdh]

In D27790#623219, @mehdi_amini wrote:

In D27790#623209, @danielcdh wrote:

Considering sample pgo profile as a weighted-forest of inline stacks, each root corresponds to a symbol in the profile binary. The job of profile preparation is to expand the IR to resemble the hot part of the forest as in profile. More details about the sample pgo can be found in http://dl.acm.org/citation.cfm?id=2854044

At ThinLTO compile step, not all nodes is expandable as they could come from other modules. That's why we need to have SampleProfileLoader invoked after all nodes are visible.

I have to read the citation to understand :)

In Sample PGO, the change of profile is far less frequent than source code. But once the profile is changed, the incremental build will be broken: everything needs to be built from scratch. I guess the same applies to instrumentation based pgo?

I don't think so: for instrumentation based PGO is applied during the compile phase (there is certainly a key difference that does not make it possible with the sample based one).

Yes, it's in compile phase, but if the profile changes, the profile summary is very likely to change, which makes it necessary to recompile all functions unless the function has no profile at all. Or am I missing something?

[mehdi_amini]

In D27790#623220, @danielcdh wrote:

Yes, it's in compile phase, but if the profile changes, the profile summary is very likely to change, which makes it necessary to recompile all functions unless the function has no profile at all. Or am I missing something?

Oh right, I missed your point, this seems valid to me!

[tejohnson]

The change looks ok to me, but I'm concerned about having the caching broken with ThinLTO + sample PGO. At least let's get that part reviewed, so these patches can go in back to back.

[danielcdh]

Add hashing of sample profile content.

[danielcdh]

Looks pretty easy to add the hashing, so I included it in this patch.

[mehdi_amini]

LGTM.

[tejohnson]

lib/LTO/LTO.cpp

135 ↗

(On Diff #81680)

Will this be expensive, for large apps with large profiles? Since this is the same for each backend, can we compute once and pass down?

[mehdi_amini]

lib/LTO/LTO.cpp

135 ↗

(On Diff #81680)

If we go this route (and we should), then I rather do it as a separate patch and do as I suggested originally when the config was added there: have a hash method on the config itself where we would hash early the whole config part and do it once early.

[tejohnson]

lib/LTO/LTO.cpp

135 ↗

(On Diff #81680)

Agreed, all the module-independent stuff should be hashed early and passed down. I suppose this is the best approach for now - maybe slow, but safe and enables this change to go in.

We can pull it out as part of the config early hashing implementation in a separate patch.

include/llvm/LTO/Config.h

struct Config {
   /// Setting this field will replace target triples in input files with this
   /// triple.
   std::string OverrideTriple;
 
   /// Setting this field will replace unspecified target triples in input files
   /// with this triple.
   std::string DefaultTriple;
 
+  /// Sample PGO profile path.
+  std::string SampleProfile;
+
   bool ShouldDiscardValueNames = true;
   DiagnosticHandlerFunction DiagHandler;
 
   /// If this field is set, LTO will write input file paths and symbol
   /// resolutions here in llvm-lto2 command line flag format. This can be
   /// used for testing and for running the LTO pipeline outside of the linker
   /// with llvm-lto2.
   std::unique_ptr<raw_ostream> ResolutionFile;

lib/LTO/LTOBackend.cpp

static void runOldPMPasses(Config &Conf, Module &Mod, TargetMachine *TM,
   PMB.Inliner = createFunctionInliningPass();
   // Unconditionally verify input since it is not verified before this
   // point and has unknown origin.
   PMB.VerifyInput = true;
   PMB.VerifyOutput = !Conf.DisableVerify;
   PMB.LoopVectorize = true;
   PMB.SLPVectorize = true;
   PMB.OptLevel = Conf.OptLevel;
+  PMB.PGOSampleUse = Conf.SampleProfile;
   if (IsThinLTO)
     PMB.populateThinLTOPassManager(passes);
   else
     PMB.populateLTOPassManager(passes);
   passes.run(Mod);
 }
 
 bool opt(Config &Conf, TargetMachine *TM, unsigned Task, Module &Mod,

test/tools/gold/X86/Inputs/afdo.prof

+f:100:3
+ 1: 100

test/tools/gold/X86/thinlto_afdo.ll

+; Generate summary sections
+; RUN: opt -module-summary %s -o %t1.o
+; RUN: opt -module-summary %p/Inputs/thinlto.ll -o %t2.o
+
+; RUN: rm -f %t1.o.4.opt.bc
+; RUN: %gold -plugin %llvmshlibdir/LLVMgold.so \
+; RUN:    --plugin-opt=thinlto \
+; RUN:    --plugin-opt=save-temps \
+; RUN:    --plugin-opt=sample-profile=%p/Inputs/afdo.prof \
+; RUN:    --plugin-opt=jobs=1 \
+; RUN:    -shared %t1.o %t2.o -o %t3
+; RUN: opt -S %t1.o.4.opt.bc | FileCheck %s
+
+target datalayout = "e-m:e-i64:64-f80:128-n8:16:32:64-S128"
+
+; CHECK: ProfileSummary
+declare void @g(...)
+declare void @h(...)
+
+define void @f() {
+entry:
+  call void (...) @g()
+  call void (...) @h()
+  ret void
+}

tools/gold/gold-plugin.cpp

#endif
   // bitcode file's path prefix matching oldprefix with newprefix.
   static std::string thinlto_prefix_replace;
   // Optional path to a directory for caching ThinLTO objects.
   static std::string cache_dir;
   // Additional options to pass into the code generator.
   // Note: This array will contain all plugin options which are not claimed
   // as plugin exclusive to pass to the code generator.
   static std::vector<const char *> extra;
+  // Sample profile file path
+  static std::string sample_profile;
 
   static void process_plugin_option(const char *opt_)
   {
     if (opt_ == nullptr)
       return;
     llvm::StringRef opt = opt_;
 
     if (opt.startswith("mcpu=")) {
if (opt.startswith("mcpu=")) {
       if (StringRef(opt_ + 5).getAsInteger(10, Parallelism))
         message(LDPL_FATAL, "Invalid parallelism level: %s", opt_ + 5);
     } else if (opt.startswith("lto-partitions=")) {
       if (opt.substr(strlen("lto-partitions="))
               .getAsInteger(10, ParallelCodeGenParallelismLevel))
         message(LDPL_FATAL, "Invalid codegen partition level: %s", opt_ + 5);
     } else if (opt == "disable-verify") {
       DisableVerify = true;
+    } else if (opt.startswith("sample-profile=")) {
+      sample_profile= opt.substr(strlen("sample-profile="));
     } else {
       // Save this option to pass to the code generator.
       // ParseCommandLineOptions() expects argv[0] to be program name. Lazily
       // add that.
       if (extra.empty())
         extra.push_back("LLVMgold");
 
       extra.push_back(opt_);
case options::OT_BC_ONLY:
     break;
 
   case options::OT_SAVE_TEMPS:
     check(Conf.addSaveTemps(output_name + ".",
                             /* UseInputModulePath */ true));
     break;
   }
 
+  if (!options::sample_profile.empty())
+    Conf.SampleProfile = options::sample_profile;
+
   return llvm::make_unique<LTO>(std::move(Conf), Backend,
                                 options::ParallelCodeGenParallelismLevel);
 }
 
 // Write empty files that may be expected by a distributed build
 // system when invoked with thinlto_index_only. This is invoked when
 // the linker has decided not to include the given module in the
 // final link. Frequently the distributed build system will want to