[PATCH] D95832: [SampleFDO][NFC] Refacrot SampleProfileLoad to reuse the code in MachineIR

[Rong Xu via Phabricator via llvm-commits]

xur added a comment.

Thanks, Wei!



================
Comment at: llvm/lib/Transforms/IPO/SampleProfile.cpp:476-477
 
+  /// Flag indicating whether input profile is context-sensitive
+  bool ProfileIsCS = false;
+
----------------
wmi wrote:
> The usages of ProfileIsCS are all in SampleProfileLoader member functions, so it doesn't need to be defined in SampleProfileLoaderImpl? 
Yes. ProfileIsCS is not longer used in base class after we break some of the methods.
Thanks for catching this. 


================
Comment at: llvm/lib/Transforms/IPO/SampleProfile.cpp:490
+  // attribute.
+  bool ProfAccForSymsInList;
+};
----------------
wmi wrote:
> Will FSAFDO need ProfAccForSymsInList to know which function is new? If not, we can leave it in SampleLoader class.
Is this only used in inline?
I'm not sure if this flag will affect the branch probability so I put into base to be safe, and I have it in FSAFDO.

If this only intends for using in inline, I can move it to base.


================
Comment at: llvm/lib/Transforms/IPO/SampleProfile.cpp:852-854
   const FunctionSamples *FS = findFunctionSamples(Inst);
   if (!FS)
     return std::error_code();
----------------
wmi wrote:
> Why not remove this part and just call SampleProfileLoaderBaseImpl::getInstWeight below instead of SampleProfileLoaderBaseImpl::getInstWeightImpl?
If we move this part down and combine the call to getInstrWeightImpl, the semantic is not the same. 
I did not look deep into this, but instead of returning error_code(), we could get 0 (and calls to findCalleeFunctionSamples().




================
Comment at: llvm/test/Transforms/SampleProfile/inline-coverage.ll:34
 
+; CHECK: remark: coverage.cc:10:16: most popular destination for conditional branches at coverage.cc:9:3
+
----------------
wmi wrote:
> Why does this CHECK need to be moved lower?
I did not dig into this. The calls to remark are in the same order. But diagnostic prints different order. I don't think this matter too much. Isn't it.


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

https://reviews.llvm.org/D95832