[PATCH] D140949: [MemProf] Context disambiguation cloning pass [patch 2/3]

[Snehasish Kumar via Phabricator via llvm-commits]

snehasish accepted this revision.
snehasish added a comment.
This revision is now accepted and ready to land.

lgtm



================
Comment at: llvm/lib/Transforms/IPO/MemProfContextDisambiguation.cpp:1994
+  // that is arbitrary, and we assert in that case below.
+  constexpr unsigned AllocTypeCloningPriority[] = {/*None*/ 3, /*NotCold*/ 4,
+                                                   /*Cold*/ 1,
----------------
tejohnson wrote:
> snehasish wrote:
> > I think EnumeratedArray [1] is a good fit for this use case. I think it would also provide some checks against out of bounds access if the AllocType enum class changed.
> > 
> > [1] llvm-project/llvm/include/llvm/ADT/EnumeratedArray.h
> I don't think an EnumeratedArray will work, because index 3 is not an actual AllocationType enum value - it is the OR of the NotCold and Cold enumeration values (AllocTypes is a uint8_t as a result). As an aside I did not want to add it as an enum value because as we extend AllocationTypes with different types (as documented in the definition, all need to be powers of 2 to allow the ORing) you could presumably have various combinations, and it didn't seem practical to encode all of them in the enum itself.
> 
> Instead I added an "All" type to the enum that is documented as needing to be the OR of all values, and I use that in an assert here.
Sounds good, thanks for the explanation.


================
Comment at: llvm/lib/Transforms/IPO/MemProfContextDisambiguation.cpp:1998
+  std::stable_sort(Node->CallerEdges.begin(), Node->CallerEdges.end(),
+                   [&](const std::shared_ptr<ContextEdge> &A,
+                       const std::shared_ptr<ContextEdge> &B) {
----------------
tejohnson wrote:
> snehasish wrote:
> > We only need to capture AllocTypeCloningPriority here. Do you know if a more permissive capture has any overhead?
> I don't believe it adds overhead, just expresses intent to the frontend. In any case, turns out constexpr don't need to be captured, so I could remove the "&" completely.
TIL that constexpr doesn't need to be captured, makes sense in retrospect.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D140949