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

Tue Aug 30 10:15:18 PDT 2022

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

================
Comment at: llvm/lib/Transforms/Instrumentation/PGOInstrumentation.cpp:1252
+                                AllocInfo->Info.getMinLifetime());
+  SmallVector<uint64_t> StackIds;
+  std::set<hash_code> StackHashSet;
----------------
snehasish wrote:
> 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.
Noted, but moot as this has been removed (see below)

================
Comment at: llvm/lib/Transforms/Instrumentation/PGOInstrumentation.cpp:1253
+  SmallVector<uint64_t> StackIds;
+  std::set<hash_code> StackHashSet;
+  for (auto StackFrame : AllocInfo->CallStack) {
----------------
snehasish wrote:
> 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.
Although this std::set has been removed, I use it elsewhere so added the include.

================
Comment at: llvm/lib/Transforms/Instrumentation/PGOInstrumentation.cpp:1256
+    auto StackId = computeStackId(StackFrame);
+    // Remove recursion cycles.
+    // TODO: Consider handling this during profile generation.
----------------
I have removed this handling. It is not correct in the case of mutual recursion involving more than one function where it will result in non-sensical stack traces. I am deferring handling recursion until later during the LTO phase.

================
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();
----------------
snehasish wrote:
> 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.
I could do this, but as is it mirrors the structure of the similar handling in readCounters, which has some advantages. wdyt?

================
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());
----------------
snehasish wrote:
> LocHashToCallSiteFrame to indicate the value in the map corresponds to an individual frame?
The key is the location hash (stack id) of a single frame, but the value is a set of all the CallSites from the profile that reference it.

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

================
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
----------------
snehasish wrote:
> Can you add an assert for this?
In this case "may only" meant "might only", not "may only at most". So I can't assert on anything. This can happen for example if we have a location that corresponds to both an allocation call and another callsite (I've seen this periodically, and can reproduce e.g. with a macro). We would need to use discriminators more widely to better distinguish them in that case (with the handling here we will only match to the allocation call for now - edit: a slight change noted further below ensures this is the case). Will change /may/might/ and add a note.

================
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
----------------
snehasish wrote:
> "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;
> ...
> ```
As noted earlier, it might be in more than one map. But I realized we could sometimes add the callsite metadata, instead of the memprof metadata, to a non-new allocation call (e.g. malloc) when there is a matching location in both maps given the structuring of the handling below. I've changed it so we handle all instructions with matching allocation profile data in the below if statement, and skip adding any metadata if there is matching allocation profile data but it is not isNewLikeFn. I've made the allocation profile matching if statement below have a continue at the end, so that I can remove the indentation further below for the callsite-only matched situation and added an assert there.

================
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
----------------
snehasish wrote:
> --check-prefixes=MEMPROF,ALL can be used instead.
Done here and elsewhere.

================
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
----------------
snehasish wrote:
> 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
> ```
It doesn't group them by prefix, but you are right there are a lot of redundant checks in this test. And one that is not correct (see below). I have cleaned this up

================
Comment at: llvm/test/Transforms/PGOProfile/memprof.ll:115
+
+;; Feed back memprof-only profile
+; RUN: opt < %s -passes=pgo-instr-use -pgo-test-profile-file=%t.pgomemprofdata -pgo-warn-missing-function -S 2>&1 | FileCheck %s --check-prefix=MEMPROF --check-prefix=PGO --check-prefix=ALL
----------------
The comment here and below and one of the checks is incorrect. This case is testing a pgo+memprof profile.

================
Comment at: llvm/test/Transforms/PGOProfile/memprof.ll:123
+; There should not be any PGO metadata
+; MEMPROF-NOT: !prof
+
----------------
This is not correct for this test. But worked because it matched the first !prof against the first PGO label below after ALL-LABEL. This was just uselessly checking that there were no additional !prof before that. I have removed this and made the earlier similar check a MEMPROFONLY check.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D128142