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

[Teresa Johnson via Phabricator via llvm-commits]

tejohnson added inline comments.


================
Comment at: llvm/lib/ProfileData/RawMemProfReader.cpp:183
+bool RawMemProfReader::hasFormat(const StringRef Path) {
+  auto BufferOr = MemoryBuffer::getFileOrSTDIN(Path, /*IsText=*/true);
+  if (!BufferOr)
----------------
snehasish wrote:
> tejohnson wrote:
> > Why IsText = true for the raw file? Not sure of the implications.
> Good catch, this param controls whether line endings are detected when the file is read [1]. Since we are working with binary files it doesn't seem to have any impact though. Updated to remove the param to default to false.
> 
> [1] https://github.com/llvm/llvm-project/blob/345223a7be3c3c93a10f8453d96287a3a03b6754/llvm/lib/Support/MemoryBuffer.cpp#L254
I see the second param removed only for the call to getFileOrSTDIN in create(), not here in hasFormat().


================
Comment at: llvm/test/tools/llvm-profdata/memprof-basic.test:61
+CHECK-NEXT:     MemInfoBlock:
+CHECK-NEXT:       alloc_count: 1
+CHECK-NEXT:       total_access_count: 0
----------------
Do these lines need to be updated to reflect D117256?


================
Comment at: llvm/test/tools/llvm-profdata/memprof-basic.test:70
+CHECK-NEXT:       dealloc_timestamp: 987
+CHECK-NEXT:       total_lifetime: 987
+CHECK-NEXT:       min_lifetime: 987
----------------
snehasish wrote:
> davidxl wrote:
> > unit of the time?
> The timestamp should be ms. Let me look into this a bit more.
The runtime reports the lifetimes in ms


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D116784