[PATCH] D116784: [ProfileData] Read and symbolize raw memprof profiles.

[Teresa Johnson via Phabricator via llvm-commits]

tejohnson added inline comments.


================
Comment at: llvm/include/llvm/ProfileData/MemProf.h:27
+  std::vector<Frame> CallStack;
+  // TODO: Replace this with the entry format described in the RFC so
+  // that the InstrProfRecord reader and writer do not have to be concerned
----------------
Can you elaborate on this and what will change?


================
Comment at: llvm/include/llvm/ProfileData/RawMemProfReader.h:80
+  Error initialize();
+  Error setupSymbolizer();
+  Error readRawProfile();
----------------
Where is this defined/called?


================
Comment at: llvm/include/llvm/ProfileData/RawMemProfReader.h:96
+  llvm::SmallVector<SegmentEntry, 16> SegmentInfo;
+  llvm::MapVector<uint64_t, MemInfoBlock> ProfileData;
+  CallStackMap StackMap;
----------------
Suggest comment on what is the map key (the uint64_t).


================
Comment at: llvm/include/llvm/ProfileData/RawMemProfReader.h:100
+  // Iterator to read from the ProfileData MapVector.
+  llvm::MapVector<uint64_t, MemInfoBlock>::iterator Next = ProfileData.end();
 };
----------------
Nit: there are a bunch of locals also named "Next" in various methods. For clarity suggest renaming this to something more unique.


================
Comment at: llvm/lib/ProfileData/RawMemProfReader.cpp:183
+bool RawMemProfReader::hasFormat(const StringRef Path) {
+  auto BufferOr = MemoryBuffer::getFileOrSTDIN(Path, /*IsText=*/true);
+  if (!BufferOr)
----------------
Why IsText = true for the raw file? Not sure of the implications.


================
Comment at: llvm/lib/ProfileData/RawMemProfReader.cpp:326
+  return object::SectionedAddress{VirtualAddress,
+                                  object::SectionedAddress::UndefSection};
+}
----------------
What is the implication of "UndefSection"?


================
Comment at: llvm/test/tools/llvm-profdata/memprof-basic.test:34
 
-RUN: llvm-profdata show --memory %p/Inputs/basic.memprofraw -o - | FileCheck %s
+RUN: llvm-profdata show --memory %p/Inputs/basic.memprofraw --profiled-binary %p/Inputs/basic.memprofexe -o - | FileCheck %s
 
----------------
I assume basic.memprofexe == rawprofile.out from above?


================
Comment at: llvm/test/tools/llvm-profdata/memprof-multi.test:36
 
-RUN: llvm-profdata show --memory %p/Inputs/multi.memprofraw -o - | FileCheck %s
+RUN: llvm-profdata show --memory %p/Inputs/multi.memprofraw --profiled-binary %p/Inputs/multi.memprofexe -o - | FileCheck %s
 
----------------
multi.memprofexe == rawprofile.out?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D116784