[PATCH] D135714: [MemProf] ThinLTO summary support
Snehasish Kumar via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Tue Oct 25 17:09:29 PDT 2022
snehasish added inline comments.
================
Comment at: llvm/include/llvm/AsmParser/LLToken.h:397
kw_varFlags,
+ kw_callsites,
+ kw_versions,
----------------
Should we add a comment indicating these are used by memprof?
================
Comment at: llvm/include/llvm/IR/ModuleSummaryIndex.h:300
+ // original callee.
+ std::vector<unsigned> Versions;
+
----------------
Call this Clones instead of Versions to avoid confusion with AllocType Version? Also see the related comment below about introducing a separate kw_clones token.
================
Comment at: llvm/include/llvm/IR/ModuleSummaryIndex.h:1335
+ unsigned addOrGetStackIdIndex(uint64_t StackId) {
+ if (StackIdToIndex.count(StackId))
+ return StackIdToIndex[StackId];
----------------
We can replace 2 lookups in the map with just one if we use `insert` and check the boolean indicating whether an insertion happened?
================
Comment at: llvm/include/llvm/IR/ModuleSummaryIndex.h:1355
+ assert(StackIdToIndex.size() == StackIds.size());
+ StackIdToIndex.clear();
+ }
----------------
Maybe call `StackIds.shrink_to_fit()` as well to trim the vector too?
================
Comment at: llvm/lib/AsmParser/LLParser.cpp:9570
+
+ std::vector<uint8_t> Versions;
+ do {
----------------
nit: Given the small number of items to parse using llvm::SmallVector would probably eliminate dynamic allocations?
================
Comment at: llvm/lib/AsmParser/LLParser.cpp:9707
+ if (parseToken(lltok::comma, "expected ',' in callsite") ||
+ parseToken(lltok::kw_versions, "expected 'versions' in callsite") ||
+ parseToken(lltok::colon, "expected ':'") ||
----------------
It would be great to use a separate token such as kw_clones here instead of reusing version for clarity. It caused a bit of confusion for me when reading the bitcode in the test.
================
Comment at: llvm/lib/Bitcode/Reader/BitcodeReader.cpp:7486
+ std::vector<unsigned> StackIdList;
+ while (I < Record.size()) {
+ assert(Record[I] < StackIds.size());
----------------
Rewriting this as `for(unsigned I = 1; I < Record.size; I++)` would reduce the scope of I a bit more and make it readable overall I think.
Alternatively consider using an iterator for Record so we don't have to worry about indices? This might be a better approach for the code below.
================
Comment at: llvm/lib/Bitcode/Writer/BitcodeWriter.cpp:3906
+template <typename Fn1, typename Fn2>
+static void
----------------
Does this need to be templated? Looks like the uses have the same signature.
L3974: Fn1 ValueInfo -> unsigned, Fn2 unsigned -> unsigned
L4434: Fn1 ValueInfo -> unsigned, Fn2 unsigned -> unsigned
================
Comment at: llvm/lib/Bitcode/Writer/BitcodeWriter.cpp:3912
+ Fn1 GetValueID, Fn2 GetStackIndex) {
+ SmallVector<uint64_t, 64> Record;
+
----------------
I think we can let the SmallVector infer the number of elements automatically unless we have a good idea of how many records we might have.
https://llvm.org/doxygen/classllvm_1_1SmallVector.html
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D135714/new/
https://reviews.llvm.org/D135714
More information about the llvm-commits
mailing list