[PATCH] D89707: [AutoFDO][llvm-profgen] Parse mmap events from perf script

[Wei Mi via Phabricator via llvm-commits]

wmi added a comment.

Lei, thanks for the patch.



================
Comment at: llvm/docs/CommandGuide/llvm-profgen.rst:14
+
+The :program:`llvm-profgen` utility generates a SPGO profile data file from 
+given perf script data files.
----------------
Please explain what SPGO represents here.


================
Comment at: llvm/tools/llvm-profgen/llvm-profgen.cpp:71
+using BinaryMap = StringMap<ProfiledBinary>;
+using BinaryAddressMap = std::map<uint64_t, ProfiledBinary *>;
+
----------------
It is a map from address to ProfiledBinary, how about using AddressBinaryMap?


================
Comment at: llvm/tools/llvm-profgen/llvm-profgen.cpp:116
+    // The binary table is currently indexed by the binary name not the full
+    // binary path. This is because the user-given path may match the one that
+    // was actually executed.
----------------
Here it is may or may not?


================
Comment at: llvm/tools/llvm-profgen/llvm-profgen.cpp:128
+    }
+    ProfiledBinary &Binary = BinaryTable[BinaryName];
+    Binary.load();
----------------
Maybe use BinaryTable.insert instead of BinaryTable.find above and use the return value to check whether the binary has been loaded before, then you don't have to search the table another time here.


================
Comment at: llvm/tools/llvm-profgen/llvm-profgen.cpp:143-144
+    // Update the binary address map.
+    BinaryAddrMap.erase(Binary.getBaseAddress());
+    BinaryAddrMap[Event.BaseAddress] = &Binary;
+
----------------
Here BinaryAddrMap needs to erase an entry before inserting a new one. I guess it is to support the case that a load image is unloaded and then reloaded at a different place. If that is correct, please add some comment to make it clear. 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D89707