[PATCH] D146452: [AutoFDO] Use flattened profiles for profile staleness metrics

[Lei Wang via Phabricator via llvm-commits]

wlei marked an inline comment as done.
wlei added inline comments.


================
Comment at: llvm/include/llvm/ProfileData/SampleProf.h:1310
 
+  static void flattenProfile(SampleProfileMap &ProfileMap) {
+    SampleProfileMap TmpProfiles;
----------------
mingmingl wrote:
> nit: const SampleProfileMap
As mentioned below, it's intentional to change the input.


================
Comment at: llvm/include/llvm/ProfileData/SampleProf.h:1313
+    flattenProfile(TmpProfiles, ProfileMap);
+    ProfileMap = std::move(TmpProfiles);
+  }
----------------
mingmingl wrote:
> nit: I'm wondering if it's simpler to pass 'ProfileMap' as opposed to use `std::move`
It's a bit tricky to update the map while iterating its element, it could insert a new FunctionSamples. We had an internal patch to do it in-place, it ended up using an additional map, later we still needed to make copies to the original ProfileMap. IMO, that's more complicated than this version. 


================
Comment at: llvm/lib/Transforms/IPO/SampleProfile.cpp:145
+    cl::desc(
+        "Use flattened profile for stale profile detection and matching."));
+
----------------
davidxl wrote:
> Can you add a test case of using the new option for state profile detection?
Test added


================
Comment at: llvm/lib/Transforms/IPO/SampleProfile.cpp:2076-2077
   if (ReportProfileStaleness || PersistProfileStaleness) {
+    if (!FlattenProfileForMatching.getNumOccurrences())
+      FlattenProfileForMatching = true;
+
----------------
mingmingl wrote:
> for my information, is it intended that flattening is true if it's not set (num occurrences = 0)? ask since it seems to change existing behavior.
Yeah, it's intended to set true, maybe I can just set the default value to true. I was thinking to use different value for pre-link and post-link here, but that's not ready, will leave it later.


================
Comment at: llvm/lib/Transforms/IPO/SampleProfile.cpp:2091-2095
+  if (FunctionSamples::ProfileIsCS) {
+    for (const auto &I : Reader.getProfiles())
+      FlatProfiles[I.second.getName()].merge(I.second);
+  } else
+    ProfileConverter::flattenProfile(FlatProfiles, Reader.getProfiles());
----------------
mingmingl wrote:
> If I read correctly, `FunctionSamples::ProfileIsCS` is a per-reader instance bool (correct me if I miss anything). So it might be useful to add tests for context-sensitive profile and non-CS profiles.
Sounds good, full CS test is added.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D146452