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

[Rong Xu via Phabricator via llvm-commits]

xur marked an inline comment as done.
xur added inline comments.


================
Comment at: llvm/lib/CodeGen/FlowSensitiveSampleProfile.cpp:110-111
+
+class FSProfileLoader final
+    : public SampleProfileLoaderBaseImpl<MachineBasicBlock> {
+public:
----------------
wenlei wrote:
> This is more like a `MIRProfileLoader` as opposed to `FSProfileLoader`. If we do IR FS profile loading, it will be handled in existing SampleLoader, the name `FSProfileLoader` would suggest this one handles all FS profile loading for both IR and MIR. Same for FSProfileLoaderPass. We could follow the naming/structure of MIRAddFSDiscriminators too and call it `MIRProfileLoader`. 
OK. I totally agree what you said. I will change the name.


================
Comment at: llvm/lib/CodeGen/FlowSensitiveSampleProfile.cpp:291
+
+bool FSProfileLoader::runOnFunction(FunctionT &F) {
+  Function &Func = F.getFunction();
----------------
wenlei wrote:
> xur wrote:
> > 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).
> Basically I was wondering why we use `runOnFunction(FunctionT &F)` instead of `runOnFunction(MachineFunction &F)` since this is only meant for MIR? The IR loader has `runOnFunction(Function &F..)`.
OK. I see what you meant. This is my bad. I should have used runOnFunction(MachineFunction &F).  Note that runOnFucntion is not a templated function: after we propagate "using FunctionT=MachineFunction", this function will become runOnFunction(MachineFunction &F). But I agree this is confusing. I will fix. Thanks!


================
Comment at: llvm/lib/CodeGen/FlowSensitiveSampleProfile.cpp:293
+  Function &Func = F.getFunction();
+  clearFunctionData(false);
+  Samples = Reader->getSamplesFor(Func);
----------------
wenlei wrote:
> xur wrote:
> > 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)
> This is still all per-function info, so it needs to be cleared before we use the profile loader to process the next function, right? or did i miss anything?
We initialize these data structures per function in setInitVals(). Each function will have its own value and no need to clear. 


================
Comment at: llvm/lib/Passes/PassBuilder.cpp:1121
+      // Set FSProfileFile so that CodeGen can read the profile.
+      FSProfileFile.setValue(PGOOpt->ProfileFile);
+    }
----------------
hoy wrote:
> xur wrote:
> > 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.
> Yes, that's what I meant. Currently `FSProfileFile` is set in postlink for LTO. It shouldn't matter if it is always set, since no prelink pass consumes that now?
Got it. I'll try to fix this.


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

https://reviews.llvm.org/D107878