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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
