[PATCH] D128142: [MemProf] Memprof profile matching and annotation

[Snehasish Kumar via Phabricator via llvm-commits]

snehasish added inline comments.
Herald added a subscriber: mingmingl.


================
Comment at: llvm/lib/Transforms/Instrumentation/PGOInstrumentation.cpp:1249
+void addCallStack(CallStackTrie &AllocTrie, const AllocationInfo *AllocInfo) {
+  auto AllocType = getAllocType(AllocInfo->Info.getMaxAccessCount(),
+                                AllocInfo->Info.getMinSize(),
----------------
Prefer moving this code after the loop, close to where AllocType is used.


================
Comment at: llvm/lib/Transforms/Instrumentation/PGOInstrumentation.cpp:1252
+                                AllocInfo->Info.getMinLifetime());
+  SmallVector<uint64_t> StackIds;
+  std::set<hash_code> StackHashSet;
----------------
I think if you use an llvm::SetVector here instead then you don't need the StackHashSet std::set below. CallstackTrie::addCallstack already accepts an ArrayRef so it won't need to change if we use a SetVector.


================
Comment at: llvm/lib/Transforms/Instrumentation/PGOInstrumentation.cpp:1253
+  SmallVector<uint64_t> StackIds;
+  std::set<hash_code> StackHashSet;
+  for (auto StackFrame : AllocInfo->CallStack) {
----------------
nit: It doesn't look like we #include <set> in this file so we are probably relying on having it transitively being included from somewhere.


================
Comment at: llvm/lib/Transforms/Instrumentation/PGOInstrumentation.cpp:1276
+  if (Error E = MemProfResult.takeError()) {
+    handleAllErrors(std::move(E), [&](const InstrProfError &IPE) {
+      auto Err = IPE.get();
----------------
Consider defining the lambda outside above the condition to reduce indentation. IMO it will be  a little easier to follow if it wasn't inlined into the if statement itself.


================
Comment at: llvm/lib/Transforms/Instrumentation/PGOInstrumentation.cpp:1297
+
+      std::string Msg = IPE.message() + std::string(" ") + F.getName().str() +
+                        std::string(" Hash = ") +
----------------
Is an llvm::Twine a better choice here instead of std::string? I guess it doesn't matter much in error handling code.  


================
Comment at: llvm/lib/Transforms/Instrumentation/PGOInstrumentation.cpp:1313
+  std::map<uint64_t, std::set<std::pair<const SmallVector<Frame> *, unsigned>>>
+      LocHashToCallSites;
+  const auto MemProfRec = std::move(MemProfResult.get());
----------------
LocHashToCallSiteFrame to indicate the value in the map corresponds to an individual frame?


================
Comment at: llvm/lib/Transforms/Instrumentation/PGOInstrumentation.cpp:1319
+    // of call stack frames.
+    auto StackId = computeStackId(AI.CallStack[0]);
+    LocHashToAllocInfo[StackId].insert(&AI);
----------------
Not using auto over here would be helpful to know that we are indexing into the map below using an uint64_t. Same below.


================
Comment at: llvm/lib/Transforms/Instrumentation/PGOInstrumentation.cpp:1330
+      // Once we find this function, we can stop recording.
+      if (StackFrame.Function == FuncGUID)
+        break;
----------------
Should we assert that it was actually found?


================
Comment at: llvm/lib/Transforms/Instrumentation/PGOInstrumentation.cpp:1360
+      // If leaf was found in a map, iterators pointing to its location in both
+      // of the maps (it may only exist in one).
+      std::map<uint64_t, std::set<const AllocationInfo *>>::iterator
----------------
Can you add an assert for this?


================
Comment at: llvm/lib/Transforms/Instrumentation/PGOInstrumentation.cpp:1365
+                                            unsigned>>>::iterator CallSitesIter;
+      for (const DILocation *DIL = I.getDebugLoc(); DIL;
+           DIL = DIL->getInlinedAt()) {
----------------
`DIL != nullptr` is a little easier to follow.


================
Comment at: llvm/lib/Transforms/Instrumentation/PGOInstrumentation.cpp:1397
+      // be non-zero.
+      auto StackFrameIncludesInlinedCallStack =
+          [&InlinedCallStack](ArrayRef<Frame> ProfileCallStack,
----------------
Prefer moving this to a static helper method to reduce the size of the loop body, reduce indentation for this logic and make it more readable overall. Probably creating an functor object on the stack for each instruction that we process is not efficient either.


================
Comment at: llvm/lib/Transforms/Instrumentation/PGOInstrumentation.cpp:1414
+
+      // First add !memprof metadata from allocation info, if we found the
+      // instruction's leaf location in that map, and if the rest of the
----------------
"First add !memprof metadata  ..." -- the ordering of the if-else condition isn't necessary though since only one of the iters can be non-null? We could rewrite the else condition first to reduce the complexity here a bit. Eg --

```
if (CallSitesIter != LocHashToCallSites.end()) {
  ...
  continue
}

// Flip the conditions here
if (!isNewLikeFn() || AllocInfoIter == LocHashToAllocInfo.end()) {
   continue
}

CallStackTrie AllocTrie;
...
```


================
Comment at: llvm/test/Transforms/PGOProfile/memprof.ll:82
+;; 	memprof.cc -o pgo.exe -fprofile-generate=.
+;; $ pgo.exe
+;; $ mv default_*.profraw memprof_pgo.profraw
----------------
./pgo.exe


================
Comment at: llvm/test/Transforms/PGOProfile/memprof.ll:95
+;; Feed back memprof-only profile
+; RUN: opt < %s -passes=pgo-instr-use -pgo-test-profile-file=%t.memprofdata -pgo-warn-missing-function -S 2>&1 | FileCheck %s --check-prefix=MEMPROF --check-prefix=ALL
+;; All memprof functions should be found
----------------
--check-prefixes=MEMPROF,ALL can be used instead.


================
Comment at: llvm/test/Transforms/PGOProfile/memprof.ll:108
+;; profile messages)
+; ALL-NOT: memprof record not found for function hash
+;; All pgo functions should be found
----------------
I suspect that the check lines are redundant. I think FileCheck scans the entire file and groups conditions by prefix. So we could have the 3 run lines followed by a group of prefix checks.

```
; ALL-NOT: memprof record not found for function hash
; ALL-NOT: no profile data available for function
; MEMPROF-NOT: !prof
; PGOONLY-NOT: !memprof
; PGOONLY-NOT: !callsite
```


================
Comment at: llvm/test/Transforms/PGOProfile/memprofmissingfunc.ll:13
+
+; CHECK: memprof record not found for function hash 10477964663628735180 _Z16funcnotinprofilev
+
----------------
Should we use a regex here to make it more resilient since we don't care about the exact hash?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D128142