[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:1313
+    flattenProfile(TmpProfiles, ProfileMap);
+    ProfileMap = std::move(TmpProfiles);
+  }
----------------
wlei wrote:
> 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. 
thanks! It's true that updating map while iterating the elements are complicated; and the current way of writing looks reasonable then (I tend to be cautious when seeing std::move but obviously over cautious here :))


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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D146452