[PATCH] D58832: [SampleFDO] add suffix elision control for fcn names

[Mircea Trofin via Phabricator via llvm-commits]

mtrofin added inline comments.


================
Comment at: include/llvm/ProfileData/SampleProf.h:415
+  /// suffix elision policy attributes.
+  static StringRef CanonicalFnName(const Function &F) {
+    auto attrName = "sample-profile-suffix-elision-policy";
----------------
Nit: getCanonicalFunctionName


================
Comment at: include/llvm/ProfileData/SampleProf.h:416
+  static StringRef CanonicalFnName(const Function &F) {
+    auto attrName = "sample-profile-suffix-elision-policy";
+    auto Attr = F.getFnAttribute(attrName).getValueAsString();
----------------
Coding style (throughout): variable names start with capital letter, e.g. AttrName, Suffix, etc.


================
Comment at: include/llvm/ProfileData/SampleProf.h:427
+      auto dit = cand.rfind('.');
+      if (dit == it + suffix.size()-1)
+        return cand.substr(0, it);
----------------
please run clang-format (there should be spaces surrounding the '-' sign)


================
Comment at: unittests/ProfileData/SampleProfTest.cpp:203
+
+  void addFcnSamples(StringMap<FunctionSamples> *smap,
+                     const char *fname,
----------------
addFunctionSamples

also, paremeters should be capitalized (SMap, FName, TotalSamples, HeadSamples)


================
Comment at: unittests/ProfileData/SampleProfTest.cpp:228
+
+  void mkModFcn(Module *M, const char *fname, StringRef policy) {
+    FunctionType *fn_type =
----------------
For clarity, please give this a more verbose name. If I understand it correctly, this is (for example) ensureSampleProfileSuffixElisionPolicy




Repository:
  rL LLVM

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

https://reviews.llvm.org/D58832