[PATCH] D97623: [SampleFDO] Add a cutoff flag to control how many symbols will be included into profile symbol list.

[Wei Mi via Phabricator via llvm-commits]

wmi added inline comments.


================
Comment at: llvm/lib/ProfileData/SampleProf.cpp:34
+static cl::opt<uint64_t> ProfileSymbolListCutOff(
+    "profile-symbol-list-cut-off", cl::Hidden, cl::init(-1), cl::ZeroOrMore,
+    cl::desc("Cutoff value about how many symbols in profile symbol list "
----------------
tejohnson wrote:
> Nit: suggest making "cutoff" one word not hyphenated in option.
Fixed.


================
Comment at: llvm/test/Transforms/SampleProfile/profile-sample-accurate.ll:66
+; Check -profile-symbol-list-cut-off=2 will include _Z3toov into profile
+; symbol list and -profile-symbol-list-cut-off=3 will not.
+; PSLCUTOFF2: define i32 @_Z3toov{{.*}}!prof ![[TOO_ID:[0-9]+]]
----------------
davidxl wrote:
> tejohnson wrote:
> > Shouldn't it be the other way around - i.e. with the larger cutoff it will be included and with the smaller one it isn't?
> > 
> > Also - confused at how the cutoff correlates to the placement in profile-symbol-list.text. In that file, _Z3toov is symbol number 5. So wouldn't a cutoff of 4 vs 5 affect whether it is included?
> I think it is sorted list.
> 
> When cutoff is 3, precise symbol list includes too, so too has profile of 0.
Nice catch. It should be the other way around. I fix it.

Yes, the order is confusing. David's explanation is right. It is sorted when the text profile symbol list is written into binary section. I change the order in the text to remove the confusion. 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D97623