[SampleFDO] add suffix elision control for fcn names

[thanm]

Summary

Add hooks for determining the policy used to decide whether/how
to chop off symbol 'suffixes' when locating a given function
in a sample profile.

Prior to this change, any function symbols of the form "X.Y" were
elided/truncated into just "X" when looking up things in a sample
profile data file.

With this change, the policy on suffixes can be changed by adding a
new attribute "sample-profile-suffix-elision-policy" to the function:
this attribute can have the value "all" (the default), "selected", or
"none". A value of "all" preserves the previous behavior (chop off
everything after the first "." character, then treat that as the
symbol name). A value of "selected" chops off only the rightmost
".llvm.XXXX" suffix (where "XXX" is any string not containing a "."
char). A value of "none" indicates that names should be left as is.

[mtrofin]

include/llvm/ProfileData/SampleProf.h

415 ↗

(On Diff #188926)

Nit: getCanonicalFunctionName

416 ↗

(On Diff #188926)

Coding style (throughout): variable names start with capital letter, e.g. AttrName, Suffix, etc.

427 ↗

(On Diff #188926)

please run clang-format (there should be spaces surrounding the '-' sign)

unittests/ProfileData/SampleProfTest.cpp

203 ↗

(On Diff #188926)

addFunctionSamples

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

228 ↗

(On Diff #188926)

For clarity, please give this a more verbose name. If I understand it correctly, this is (for example) ensureSampleProfileSuffixElisionPolicy

[thanm]

unittests/ProfileData/SampleProfTest.cpp

228 ↗

(On Diff #188926)

OK. I've renamed to createFunctionWithSampleProfileElisionPolicy

[thanm]

Formatting and variable/function naming.

[wmi]

We need function attribute only when we want to use different strategies for different functions. Do we need the function granularity control over the suffix elision policy?

[thanm]

In D58832#1422205, @wmi wrote:

We need function attribute only when we want to use different strategies for different functions. Do we need the function granularity control over the suffix elision policy?

We want to be able to eventually support ThinLTO in which you have a mix of C and Go functions (it is fairly common for Go programs to call out into C code). Without such a change we'd have to pick one policy or another and hope that it works for code generated by every front end.

[wmi]

In D58832#1423171, @thanm wrote:

In D58832#1422205, @wmi wrote:

We need function attribute only when we want to use different strategies for different functions. Do we need the function granularity control over the suffix elision policy?

We want to be able to eventually support ThinLTO in which you have a mix of C and Go functions (it is fairly common for Go programs to call out into C code). Without such a change we'd have to pick one policy or another and hope that it works for code generated by every front end.

That makes sense to me. Thanks for explaining.

[wmi]

include/llvm/ProfileData/SampleProf.h

420–429 ↗

(On Diff #189511)

It may be better to use a suffix array here so that it is easier for others to add new suffix later.

433 ↗

(On Diff #189511)

Add an error message here: assert(false && "xxx")

include/llvm/ProfileData/SampleProfReader.h

289–290 ↗

(On Diff #189511)

Now that the function names are not necessarily all stripped, we want to update the comment accordingly.

[thanm]

include/llvm/ProfileData/SampleProfReader.h

289–290 ↗

(On Diff #189511)

I've updated the comment.

[thanm]

Additional changes suggested by Wei.

[wmi]

LGTM. Thanks for working on it!

[thanm]

Thanks for reviewing.

llvm/trunk/include/llvm/ProfileData/SampleProf.h

public:
   /// Return the function name.
   StringRef getName() const { return Name; }
 
   /// Return the original function name if it exists in Module \p M.
   StringRef getFuncNameInModule(const Module *M) const {
     return getNameInModule(Name, M);
   }
 
+  /// Return the canonical name for a function, taking into account
+  /// suffix elision policy attributes.
+  static StringRef getCanonicalFnName(const Function &F) {
+    static const char *knownSuffixes[] = { ".llvm.", ".part." };
+    auto AttrName = "sample-profile-suffix-elision-policy";
+    auto Attr = F.getFnAttribute(AttrName).getValueAsString();
+    if (Attr == "" || Attr == "all") {
+      return F.getName().split('.').first;
+    } else if (Attr == "selected") {
+      StringRef Cand(F.getName());
+      for (const auto &Suf : knownSuffixes) {
+        StringRef Suffix(Suf);
+        auto It = Cand.rfind(Suffix);
+        if (It == StringRef::npos)
+          return Cand;
+        auto Dit = Cand.rfind('.');
+        if (Dit == It + Suffix.size() - 1)
+          Cand = Cand.substr(0, It);
+      }
+      return Cand;
+    } else if (Attr == "none") {
+      return F.getName();
+    } else {
+      assert(false && "internal error: unknown suffix elision policy");
+    }
+    return F.getName();
+  }
+
   /// Translate \p Name into its original name in Module.
   /// When the Format is not SPF_Compact_Binary, \p Name needs no translation.
   /// When the Format is SPF_Compact_Binary, \p Name in current FunctionSamples
   /// is actually GUID of the original function name. getNameInModule will
   /// translate \p Name in current FunctionSamples into its original name.
   /// If the original name doesn't exist in \p M, return empty StringRef.
   StringRef getNameInModule(StringRef Name, const Module *M) const {
     if (Format != SPF_Compact_Binary)
GUIDToFuncNameMapper(Module &M) {
         GUIDToFuncNameMap.insert({Function::getGUID(OrigName), OrigName});
         /// Local to global var promotion used by optimization like thinlto
         /// will rename the var and add suffix like ".llvm.xxx" to the
         /// original local name. In sample profile, the suffixes of function
         /// names are all stripped. Since it is possible that the mapper is
         /// built in post-thin-link phase and var promotion has been done,
         /// we need to add the substring of function name without the suffix
         /// into the GUIDToFuncNameMap.
-        auto pos = OrigName.find('.');
-        if (pos != StringRef::npos) {
-          StringRef NewName = OrigName.substr(0, pos);
-          GUIDToFuncNameMap.insert({Function::getGUID(NewName), NewName});
-        }
+        StringRef CanonName = getCanonicalFnName(F);
+        if (CanonName != OrigName)
+          GUIDToFuncNameMap.insert({Function::getGUID(CanonName), CanonName});
       }
       CurrentModule = &M;
     }
 
     ~GUIDToFuncNameMapper() {
       if (Format != SPF_Compact_Binary)
         return;
 

llvm/trunk/include/llvm/ProfileData/SampleProfReader.h

public:
 
   virtual void collectFuncsToUse(const Module &M) {}
 
   /// Print all the profiles on stream \p OS.
   void dump(raw_ostream &OS = dbgs());
 
   /// Return the samples collected for function \p F.
   FunctionSamples *getSamplesFor(const Function &F) {
-    // The function name may have been updated by adding suffix. In sample
-    // profile, the function names are all stripped, so we need to strip
-    // the function name suffix before matching with profile.
-    return getSamplesFor(F.getName().split('.').first);
+    // The function name may have been updated by adding suffix. Call
+    // a helper to (optionally) strip off suffixes so that we can
+    // match against the original function name in the profile.
+    StringRef CanonName = FunctionSamples::getCanonicalFnName(F);
+    return getSamplesFor(CanonName);
   }
 
   /// Return the samples collected for function \p F.
   virtual FunctionSamples *getSamplesFor(StringRef Fname) {
     std::string FGUID;
     Fname = getRepInFormat(Fname, getFormat(), FGUID);
     auto It = Profiles.find(Fname);
     if (It != Profiles.end())

llvm/trunk/lib/ProfileData/SampleProfReader.cpp

std::error_code SampleProfileReaderCompactBinary::readFuncOffsetTable() {
   End = TableStart;
   Data = SavedData;
   return sampleprof_error::success;
 }
 
 void SampleProfileReaderCompactBinary::collectFuncsToUse(const Module &M) {
   FuncsToUse.clear();
   for (auto &F : M) {
-    StringRef Fname = F.getName().split('.').first;
-    FuncsToUse.insert(Fname);
+    StringRef CanonName = FunctionSamples::getCanonicalFnName(F);
+    FuncsToUse.insert(CanonName);
   }
 }
 
 std::error_code SampleProfileReaderBinary::readSummaryEntry(
     std::vector<ProfileSummaryEntry> &Entries) {
   auto Cutoff = readNumber<uint64_t>();
   if (std::error_code EC = Cutoff.getError())
     return EC;

llvm/trunk/unittests/ProfileData/SampleProfTest.cpp

void testRoundTrip(SampleProfileFormat Format, bool Remap) {
     M.setProfileSummary(MD, ProfileSummary::PSK_Sample);
     MD = M.getProfileSummary(/* IsCS */ false);
     ASSERT_TRUE(MD);
     PS = ProfileSummary::getFromMD(MD);
     ASSERT_TRUE(PS);
     VerifySummary(*PS);
     delete PS;
   }
+
+  void addFunctionSamples(StringMap<FunctionSamples> *Smap, const char *Fname,
+                          uint64_t TotalSamples, uint64_t HeadSamples) {
+    StringRef Name(Fname);
+    FunctionSamples FcnSamples;
+    FcnSamples.setName(Name);
+    FcnSamples.addTotalSamples(TotalSamples);
+    FcnSamples.addHeadSamples(HeadSamples);
+    FcnSamples.addBodySamples(1, 0, HeadSamples);
+    (*Smap)[Name] = FcnSamples;
+  }
+
+  StringMap<FunctionSamples> setupFcnSamplesForElisionTest(StringRef Policy) {
+    StringMap<FunctionSamples> Smap;
+    addFunctionSamples(&Smap, "foo", uint64_t(20301), uint64_t(1437));
+    if (Policy == "" || Policy == "all")
+      return Smap;
+    addFunctionSamples(&Smap, "foo.bar", uint64_t(20303), uint64_t(1439));
+    if (Policy == "selected")
+      return Smap;
+    addFunctionSamples(&Smap, "foo.llvm.2465", uint64_t(20305), uint64_t(1441));
+    return Smap;
+  }
+
+  void createFunctionWithSampleProfileElisionPolicy(Module *M,
+                                                    const char *Fname,
+                                                    StringRef Policy) {
+    FunctionType *FnType =
+        FunctionType::get(Type::getVoidTy(Context), {}, false);
+    auto Inserted = M->getOrInsertFunction(Fname, FnType);
+    auto Fcn = cast<Function>(Inserted.getCallee());
+    if (Policy != "")
+      Fcn->addFnAttr("sample-profile-suffix-elision-policy", Policy);
+  }
+
+  void setupModuleForElisionTest(Module *M, StringRef Policy) {
+    createFunctionWithSampleProfileElisionPolicy(M, "foo", Policy);
+    createFunctionWithSampleProfileElisionPolicy(M, "foo.bar", Policy);
+    createFunctionWithSampleProfileElisionPolicy(M, "foo.llvm.2465", Policy);
+  }
+
+  void testSuffixElisionPolicy(SampleProfileFormat Format, StringRef Policy,
+                               const StringMap<uint64_t> &Expected) {
+    SmallVector<char, 128> ProfilePath;
+    std::error_code EC;
+    EC = llvm::sys::fs::createTemporaryFile("profile", "", ProfilePath);
+    ASSERT_TRUE(NoError(EC));
+    StringRef ProfileFile(ProfilePath.data(), ProfilePath.size());
+
+    Module M("my_module", Context);
+    setupModuleForElisionTest(&M, Policy);
+    StringMap<FunctionSamples> ProfMap = setupFcnSamplesForElisionTest(Policy);
+
+    // write profile
+    createWriter(Format, ProfileFile);
+    EC = Writer->write(ProfMap);
+    ASSERT_TRUE(NoError(EC));
+    Writer->getOutputStream().flush();
+
+    // read profile
+    readProfile(M, ProfileFile);
+    EC = Reader->read();
+    ASSERT_TRUE(NoError(EC));
+
+    for (auto I = Expected.begin(); I != Expected.end(); ++I) {
+      uint64_t Esamples = uint64_t(-1);
+      FunctionSamples *Samples = Reader->getSamplesFor(I->getKey());
+      if (Samples != nullptr)
+        Esamples = Samples->getTotalSamples();
+      ASSERT_EQ(I->getValue(), Esamples);
+    }
+  }
 };
 
 TEST_F(SampleProfTest, roundtrip_text_profile) {
   testRoundTrip(SampleProfileFormat::SPF_Text, false);
 }
 
 TEST_F(SampleProfTest, roundtrip_raw_binary_profile) {
   testRoundTrip(SampleProfileFormat::SPF_Binary, false);
TEST_F(SampleProfTest, sample_overflow_saturation) {
 
   Result = FooSamples.addBodySamples(10, 0, Max);
   ASSERT_EQ(Result, sampleprof_error::counter_overflow);
   ErrorOr<uint64_t> BodySamples = FooSamples.findSamplesAt(10, 0);
   ASSERT_FALSE(BodySamples.getError());
   ASSERT_EQ(BodySamples.get(), Max);
 }
 
+TEST_F(SampleProfTest, default_suffix_elision_text) {
+  // Default suffix elision policy: strip everything after first dot.
+  // This implies that all suffix variants will map to "foo", so
+  // we don't expect to see any entries for them in the sample
+  // profile.
+  StringMap<uint64_t> Expected;
+  Expected["foo"] = uint64_t(20301);
+  Expected["foo.bar"] = uint64_t(-1);
+  Expected["foo.llvm.2465"] = uint64_t(-1);
+  testSuffixElisionPolicy(SampleProfileFormat::SPF_Text, "", Expected);
+}
+
+TEST_F(SampleProfTest, default_suffix_elision_compact_binary) {
+  // Default suffix elision policy: strip everything after first dot.
+  // This implies that all suffix variants will map to "foo", so
+  // we don't expect to see any entries for them in the sample
+  // profile.
+  StringMap<uint64_t> Expected;
+  Expected["foo"] = uint64_t(20301);
+  Expected["foo.bar"] = uint64_t(-1);
+  Expected["foo.llvm.2465"] = uint64_t(-1);
+  testSuffixElisionPolicy(SampleProfileFormat::SPF_Compact_Binary, "",
+                          Expected);
+}
+
+TEST_F(SampleProfTest, selected_suffix_elision_text) {
+  // Profile is created and searched using the "selected"
+  // suffix elision policy: we only strip a .XXX suffix if
+  // it matches a pattern known to be generated by the compiler
+  // (e.g. ".llvm.<digits>").
+  StringMap<uint64_t> Expected;
+  Expected["foo"] = uint64_t(20301);
+  Expected["foo.bar"] = uint64_t(20303);
+  Expected["foo.llvm.2465"] = uint64_t(-1);
+  testSuffixElisionPolicy(SampleProfileFormat::SPF_Text, "selected", Expected);
+}
+
+TEST_F(SampleProfTest, selected_suffix_elision_compact_binary) {
+  // Profile is created and searched using the "selected"
+  // suffix elision policy: we only strip a .XXX suffix if
+  // it matches a pattern known to be generated by the compiler
+  // (e.g. ".llvm.<digits>").
+  StringMap<uint64_t> Expected;
+  Expected["foo"] = uint64_t(20301);
+  Expected["foo.bar"] = uint64_t(20303);
+  Expected["foo.llvm.2465"] = uint64_t(-1);
+  testSuffixElisionPolicy(SampleProfileFormat::SPF_Compact_Binary, "selected",
+                          Expected);
+}
+
+TEST_F(SampleProfTest, none_suffix_elision_text) {
+  // Profile is created and searched using the "none"
+  // suffix elision policy: no stripping of suffixes at all.
+  // Here we expect to see all variants in the profile.
+  StringMap<uint64_t> Expected;
+  Expected["foo"] = uint64_t(20301);
+  Expected["foo.bar"] = uint64_t(20303);
+  Expected["foo.llvm.2465"] = uint64_t(20305);
+  testSuffixElisionPolicy(SampleProfileFormat::SPF_Text, "none", Expected);
+}
+
+TEST_F(SampleProfTest, none_suffix_elision_compact_binary) {
+  // Profile is created and searched using the "none"
+  // suffix elision policy: no stripping of suffixes at all.
+  // Here we expect to see all variants in the profile.
+  StringMap<uint64_t> Expected;
+  Expected["foo"] = uint64_t(20301);
+  Expected["foo.bar"] = uint64_t(20303);
+  Expected["foo.llvm.2465"] = uint64_t(20305);
+  testSuffixElisionPolicy(SampleProfileFormat::SPF_Compact_Binary, "none",
+                          Expected);
+}
+
 } // end anonymous namespace