[PATCH] D107878: [SampleFDO] Flow Sensitive Sample FDO (FSAFDO) profile loader

[Wenlei He via Phabricator via llvm-commits]

wenlei added inline comments.


================
Comment at: llvm/lib/CodeGen/FlowSensitiveSampleProfile.cpp:291
+
+bool FSProfileLoader::runOnFunction(FunctionT &F) {
+  Function &Func = F.getFunction();
----------------
FSProfileLoader inherits from `SampleProfileLoaderBaseImpl<MachineBasicBlock>`, so it's for MIR only, why do we still have a templated runOnFunction?

If we want something that serves both IR and MIR, perhaps it can go into base. Or more generally, how do we plan to support IR FS-loader?


================
Comment at: llvm/lib/CodeGen/FlowSensitiveSampleProfile.cpp:293
+  Function &Func = F.getFunction();
+  clearFunctionData(false);
+  Samples = Reader->getSamplesFor(Func);
----------------
Why do we keep dominator tree and loop info?


================
Comment at: llvm/lib/CodeGen/FlowSensitiveSampleProfile.cpp:305
+  // Set the new BPI, BFI.
+  if (EnableFSBranchProb)
+    setBranchProbs(F);
----------------
When do we want to run FS loader but not setting branch probabilities?


================
Comment at: llvm/lib/CodeGen/TargetPassConfig.cpp:1126
+        sampleprof::FSDiscriminatorPass::Pass1));
+  std::string FSFile = getFSProfileFile();
+  if (!FSFile.empty())
----------------
nit: for readability, the EnableFSDiscriminator check inside getFSProfileFile can be removed, and loader pass can be guarded under the same if check for disc pass. 


================
Comment at: llvm/lib/CodeGen/TargetPassConfig.cpp:1128
+  if (!FSFile.empty())
+    addPass(createFSProfileLoaderPass(FSFile,
+                                      sampleprof::FSDiscriminatorPass::Pass1));
----------------
I'm wondering if there's a need for users to control which pass to have profile loading independently, at least for tuning purpose? i.e. some switch like `disable-ra-fsprofile-load`, `disable-layout-fsprofile-load`.


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

https://reviews.llvm.org/D107878