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

[Rong Xu via Phabricator via llvm-commits]

xur marked 2 inline comments as done.
xur added a comment.

Thanks all for the review comments! My replies are inlined.



================
Comment at: llvm/lib/CodeGen/FlowSensitiveSampleProfile.cpp:34
+                           cl::init(false),
+                           cl::desc("Disable flow senstive profile loading"));
+static cl::opt<bool> EnableFSBranchProb(
----------------
hoy wrote:
> typo: senstive
Fixed


================
Comment at: llvm/lib/CodeGen/FlowSensitiveSampleProfile.cpp:35
+                           cl::desc("Disable flow senstive profile loading"));
+static cl::opt<bool> EnableFSBranchProb(
+    "enable-fs-branchprob", cl::Hidden, cl::init(true),
----------------
hoy wrote:
> Wondering why needs this switch. The whole point of `FSProfileLoader` is to overwrite branch weights?
some of the benefits are from the improved profile (i.e. using sum rather max) that helps SampleLoder (inline for example).
This option help to measure it.
I tested it in many of my experiments. But I agree that it does not need to expose to the users. 
I will remove this.


================
Comment at: llvm/lib/CodeGen/FlowSensitiveSampleProfile.cpp:291
+
+bool FSProfileLoader::runOnFunction(FunctionT &F) {
+  Function &Func = F.getFunction();
----------------
wenlei wrote:
> 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?
Not sure I follow the question here. Basically this is MIR version of runOnFunction. 
APIs etc are different from IR version of runOnFunction. If we want IR FS-loader, that should be based on SampleProfileLoader (need to remove the inline related code).


================
Comment at: llvm/lib/CodeGen/FlowSensitiveSampleProfile.cpp:293
+  Function &Func = F.getFunction();
+  clearFunctionData(false);
+  Samples = Reader->getSamplesFor(Func);
----------------
wenlei wrote:
> Why do we keep dominator tree and loop info?
Profile loader does not invalid these infos (this is different in SampleLoader, where inline will change it)


================
Comment at: llvm/lib/CodeGen/FlowSensitiveSampleProfile.cpp:305
+  // Set the new BPI, BFI.
+  if (EnableFSBranchProb)
+    setBranchProbs(F);
----------------
wenlei wrote:
> When do we want to run FS loader but not setting branch probabilities?
Sorry. This is a list minute bug (as I mentioned in answering Wei's comments)


================
Comment at: llvm/lib/CodeGen/TargetPassConfig.cpp:1126
+        sampleprof::FSDiscriminatorPass::Pass1));
+  std::string FSFile = getFSProfileFile();
+  if (!FSFile.empty())
----------------
wenlei wrote:
> 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. 
OK. I will change this.


================
Comment at: llvm/lib/CodeGen/TargetPassConfig.cpp:1128
+  if (!FSFile.empty())
+    addPass(createFSProfileLoaderPass(FSFile,
+                                      sampleprof::FSDiscriminatorPass::Pass1));
----------------
wenlei wrote:
> 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`.
Yes. that's a good suggestion. I havd them in my testing implementation but got it out when sending for review. I will add them back.


================
Comment at: llvm/lib/Passes/PassBuilder.cpp:1121
+      // Set FSProfileFile so that CodeGen can read the profile.
+      FSProfileFile.setValue(PGOOpt->ProfileFile);
+    }
----------------
hoy wrote:
> Can this be moved into the constructor of `PGOOptions` so that it looks more consistent with the rest of PGO setup?
Do you mean we still using FSProfileFile but just move the assignment to PGOOptions constructor? This should do doable.


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

https://reviews.llvm.org/D107878