[PATCH] D54819: [SampleFDO] Skip profile reading when flatten profile is used in ThinLTO postlink phase

[Teresa Johnson via Phabricator via llvm-commits]

tejohnson added inline comments.


================
Comment at: lib/Passes/PassBuilder.cpp:619
+    // no need to load the profile again in PostLink.
+    if (!FlattenedProfileUsed || Phase != ThinLTOPhase::PostLink)
+      MPM.addPass(SampleProfileLoaderPass(PGOOpt->SampleProfileFile,
----------------
Minor nit: to me this would be more understandable if written as:

if (!(FlattenedProfileUsed && Phase == ThinLTOPhase::PostLink))

(ditto for the old PM check, although I have another note down there that is more fundamental).


================
Comment at: lib/Transforms/IPO/PassManagerBuilder.cpp:438
+    // no need to load the profile again in PostLink.
+    if (!FlattenedProfileUsed || PrepareForThinLTO)
+      MPM.add(createSampleProfileLoaderPass(PGOSampleUse));
----------------
By guarding this with PrepareForThinLTO, I think that means that with a flattened profile and a non-thinlto build we would not get the sample PGO loaded. Should PrepareForThinLTO instead be !PerformThinLTO (which is equivalent to your new PM check)?


================
Comment at: tools/opt/opt.cpp:391
 
+  switch (PGOKindFlag) {
+  case InstrGen:
----------------
How have we been testing the PGO pipelines in the old PM without this change?


Repository:
  rL LLVM

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D54819/new/

https://reviews.llvm.org/D54819