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

[Teresa Johnson via Phabricator via llvm-commits]

tejohnson marked 11 inline comments as done.
tejohnson added inline comments.


================
Comment at: llvm/lib/Passes/PassBuilder.cpp:124
 #include "llvm/Transforms/IPO/OpenMPOpt.h"
+#include "llvm/Transforms/IPO/MemProfContextDisambiguation.h"
 #include "llvm/Transforms/IPO/PartialInlining.h"
----------------
snehasish wrote:
> I don't see anything used from this header in this file?
This turns out to be a merge issue. We actually do need the header (otherwise we get errors because of the use in PassRegistry.def which is included from this file), but I already have it included in the properly alphabetized location just above here at line 120. Removed this one.


================
Comment at: llvm/lib/Transforms/IPO/MemProfContextDisambiguation.cpp:1181
 
+      ContextNode *LastNode = CurNode;
+
----------------
snehasish wrote:
> I think this var shadows the LastNode var outside the loop nest. Is this intentional? Should we rename this var if not?
Not intentional, but thankfully not causing a bug. I simply removed LastNode and use CurNode directly in the one use below.


================
Comment at: llvm/lib/Transforms/IPO/MemProfContextDisambiguation.cpp:1949
+    // Skip any that have been removed by an earlier recursive call.
+    if (Edge->Callee == nullptr && Edge->Caller == nullptr)
+      continue;
----------------
snehasish wrote:
> Can we also drop this Edge from the Node->CallerEdges set if both Caller and Callee are set to null?
It would have been removed. Added assert.


================
Comment at: llvm/lib/Transforms/IPO/MemProfContextDisambiguation.cpp:1957
+
+  // Check if we reached an unambiguous call or have no more callers.
+  if (hasSingleAllocType(Node->AllocTypes) || Node->CallerEdges.size() <= 1)
----------------
snehasish wrote:
> The code below allows 1 caller? 
Fixed wording - I mean only a single caller.


================
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,
----------------
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.


================
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) {
----------------
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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D140949