[PATCH] D26370: Use max(BFI_Count, TotalProfCount) to get block's profile count.

[David Li via llvm-commits]

davidxl added inline comments.


================
Comment at: lib/Analysis/BlockFrequencyInfo.cpp:155
 Optional<uint64_t>
 BlockFrequencyInfo::getBlockProfileCount(const BasicBlock *BB) const {
   if (!BFI)
----------------
danielcdh wrote:
> davidxl wrote:
> > This does not look like the right place for the adjustment. Maybe provide a wrapper method in ProfileSummaryInfo::getBlockProfileCount ..
> > 
> > Besides, the adjustment does not work well for Instrumentation based PGO -- the branch weight may be scaled (though since it scales down, it does not matter in reality).
> My concern is that if we add ProfileSummaryInfo::getBlockProfileCount, it will get confused with BlockFrequencyInfo::getBlockProfileCount.
> 
> Do you mean that we hide this function in BlockFrequencyInfo, and only expose it in ProfileSummaryInfo, and change all callsites to PSI? It will introduce dependency to PSI (though there are not many callsites to this function)
Another reason this is probably the wrong way to handle this is because inlining/cloning operation does not actually update the weight meta data (scaling) -- using the original total weight is not correct.

Is it possible for SampleProfiler pass to identify those BBs that need special hanlding and introduce a new MD_prof data (with name "profile_count") -- the instruction cloner needs to be taught to update it properly


https://reviews.llvm.org/D26370