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

[Mingming Liu via Phabricator via llvm-commits]

mingmingl added inline comments.


================
Comment at: llvm/include/llvm/ProfileData/SampleProf.h:1310
 
+  static void flattenProfile(SampleProfileMap &ProfileMap) {
+    SampleProfileMap TmpProfiles;
----------------
nit: const SampleProfileMap


================
Comment at: llvm/include/llvm/ProfileData/SampleProf.h:1313
+    flattenProfile(TmpProfiles, ProfileMap);
+    ProfileMap = std::move(TmpProfiles);
+  }
----------------
nit: I'm wondering if it's simpler to pass 'ProfileMap' as opposed to use `std::move`


================
Comment at: llvm/lib/Transforms/IPO/SampleProfile.cpp:2076-2077
   if (ReportProfileStaleness || PersistProfileStaleness) {
+    if (!FlattenProfileForMatching.getNumOccurrences())
+      FlattenProfileForMatching = true;
+
----------------
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.


================
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());
----------------
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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D146452