[PATCH] D100528: [CSSPGO][llvm-profdata] Support trimming cold context when merging profiles

[Wei Mi via Phabricator via llvm-commits]

wmi added inline comments.


================
Comment at: llvm/tools/llvm-profdata/CMakeLists.txt:2
 set(LLVM_LINK_COMPONENTS
+  Analysis
   Core
----------------
wenlei wrote:
> wenlei wrote:
> > wmi wrote:
> > > wenlei wrote:
> > > > This is because we now use ProfileSummaryInfo.cpp, so we can get all consistent hot/cold threshold computation and related switches, though pulling in entire Analysis lib is less than ideal. I'm thinking about moving ProfileSummaryInfo into ProfileData/ProfileSummaryBuildder.cpo from Analysis/ProfileSummaryInfo.cpp. But I'd like to get feedback before I move things around. 
> > > Extract ProfileSummaryInfo from Analysis/ directory sounds reasonable to me. How about move it to Core -- lib/IR/ProfileSummary.cpp for example? The reason is ProfileSummaryInfo interface is used widely. If someday we want to use the interface in Core or Support which ProfileData depends on, it will introduce cyclic dependence. Putting it in Core will clear the problem. 
> > Sounds good. Thanks for the suggestion.
> Looks like extracting ProfileSummaryInfo to Core isn't a trivial move. ProfileSummaryInfo has a bunch of interfaces taking BFI as parameter, and also offers invalidation, so it depends on Analysis. It's fine when ProfileSummaryInfo itself is part of Analysis, but if we move it to Core, we will have cyclic dependency. For the same reason, we can't move it to ProfileData either. 
> 
> If we want to move it, we will need to break it up, so the part that does the threshold computation can be moved, and the part that does the query (relies on Analysis) stays.  
> 
> Now what I'm thinking about is only moving the threshold/cutoff switches in ProfileSummaryInfo.cpp along with part of ProfileSummaryInfo::computeThresholds into ProfileData/ProfileSummaryBuilder. That won't change dependencies as ProfileSummaryInfo uses ProfileSummaryBuilder already. But it lets tools that only uses ProfileData access consistent threshold computation too.
Ok, that sounds good to me. Thanks for trying to find a good solution. 


================
Comment at: llvm/tools/llvm-profdata/llvm-profdata.cpp:887-891
+  cl::opt<bool> SampleMergeColdContext(
+      "sample-merge-cold-context", cl::init(false), cl::Hidden,
+      cl::desc(
+          "Merge context sample profiles whose count is below cold threshold"));
+  cl::opt<bool> SampleTrimColdContext(
----------------
What will happen if these two flags are enabled for non-CSSPGO sample profile?


================
Comment at: llvm/tools/llvm-profgen/CMakeLists.txt:6
   AllTargetsInfos
+  Analysis
   Core
----------------
After the PSI switch and threshold are moved to ProfData, no need to depend on Analysis anymore? 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D100528