[PATCH] D121651: [llvm-profdata] Convert nested profile to CS flat profile.

[Wenlei He via Phabricator via llvm-commits]

wenlei added a comment.

Some linter seems legit.



================
Comment at: llvm/lib/ProfileData/SampleProf.cpp:499
 
-void CSProfileConverter::convertProfiles(CSProfileConverter::FrameNode &Node) {
+void CSProfileConverter::convertToCSFlatProfiles(CSProfileConverter::FrameNode &Node) {
   // Process each child profile. Add each child profile to callsite profile map
----------------
I'm a bit confused by the name `convertToCSFlatProfiles`, looking at the implementation isn't this converting to nested profile?


================
Comment at: llvm/lib/ProfileData/SampleProf.cpp:545
 
-void CSProfileConverter::convertProfiles() { convertProfiles(RootFrame); }
+void CSProfileConverter::convertToCSNestedProfiles() {
+  for (auto &FuncSample : ProfileMap) {
----------------
Here the caller name is `convertToCSNestedProfiles`, but the callee on line 552 is `convertToCSFlatProfiles`.


================
Comment at: llvm/test/tools/llvm-profdata/cs-sample-flat-profile.test:1
+RUN: llvm-profdata merge --sample --text -output=%t.proftext %S/Inputs/sample-profile.proftext --gen-cs-flat-profile=1
+RUN: FileCheck %s < %t.proftext
----------------
Add round trip test for conversion? 


================
Comment at: llvm/tools/llvm-profdata/llvm-profdata.cpp:735
                    StringRef ProfileSymbolListFile, bool CompressAllSections,
-                   bool UseMD5, bool GenPartialProfile, bool GenCSNestedProfile,
+                   bool UseMD5, bool GenPartialProfile, bool GenCSNestedProfile, bool GenCSFlatProfile,
                    bool SampleMergeColdContext, bool SampleTrimColdContext,
----------------
Are the two flags (GenCSNestedProfile/GenCSFlatProfile and the corresponding switches) supposed to be mutually exclusive? If yes, perhaps we should make it explicit? 

one way to make then mutual exclusive is to use a single switch: gen-cs-nested-profile==true means nested, false means flat. Or we can have `-gen-cs-profile={nest|flat}`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D121651