[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:110-111
+
+class FSProfileLoader final
+    : public SampleProfileLoaderBaseImpl<MachineBasicBlock> {
+public:
----------------
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`. 


================
Comment at: llvm/lib/CodeGen/FlowSensitiveSampleProfile.cpp:291
+
+bool FSProfileLoader::runOnFunction(FunctionT &F) {
+  Function &Func = F.getFunction();
----------------
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..)`.


================
Comment at: llvm/lib/CodeGen/FlowSensitiveSampleProfile.cpp:293
+  Function &Func = F.getFunction();
+  clearFunctionData(false);
+  Samples = Reader->getSamplesFor(Func);
----------------
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?


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

https://reviews.llvm.org/D107878