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

[Wei Mi via Phabricator via llvm-commits]

wmi added inline comments.


================
Comment at: llvm/lib/Transforms/IPO/SampleProfile.cpp:490
+  // attribute.
+  bool ProfAccForSymsInList;
+};
----------------
xur wrote:
> 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.
It is used to set function EntryCount. For old symbol (which show up in the binary where profile is collected) without profile, we set its EntryCount to 0 instead of -1, indicating it is very cold instead of unknown. It is also used in callsiteIsHot function. It won't affect branch probablity, so I think FSAFDO may not need it.   


================
Comment at: llvm/lib/Transforms/IPO/SampleProfile.cpp:852-854
   const FunctionSamples *FS = findFunctionSamples(Inst);
   if (!FS)
     return std::error_code();
----------------
xur wrote:
> 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().
> 
> 
It won't change the semantic. findCalleeFunctionSamples calls findFunctionSamples inside of it too. If findFunctionSamples returns nullptr for a call inst, findCalleeFunctionSamples will return nullptr as well, and the "return 0" branch won't be taken. 


================
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
+
----------------
xur wrote:
> 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.
It could expose some hidden behavior change somewhere so may be good to find out.


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

https://reviews.llvm.org/D95832