[PATCH] D151022: [llvm-exegesis] Introduce SubprocessMemory Utility Class

[Clement Courbet via Phabricator via llvm-commits]

courbet added inline comments.


================
Comment at: llvm/tools/llvm-exegesis/lib/BenchmarkResult.h:48
+  APInt Value;
+  size_t Size;
+  size_t Number;
----------------
Are these bytes or multiples of `Value.getWidth()`. Judgin from the code, bytes. But this should be clarified here.


================
Comment at: llvm/tools/llvm-exegesis/lib/BenchmarkResult.h:46
 
+struct MemoryValue {
+  APInt Value;
----------------
Please document these.


================
Comment at: llvm/tools/llvm-exegesis/lib/SubprocessMemory.cpp:68-70
+    // fill the last section
+    memcpy(SharedMemoryMapping + CurrentByte, MemVal.Value.getRawData(),
+           MemVal.Size - CurrentByte);
----------------
Is there a use case for fractions of the value size ?


================
Comment at: llvm/tools/llvm-exegesis/lib/SubprocessMemory.cpp:121
+Error SubprocessMemory::initializeSubprocessMemory(pid_t ProcessPID) {
+  report_fatal_error("initializeSubprocessMemory is only supported on Linux");
+}
----------------
Did you mean to return an error instead ? The function already returns an error, we don;t need to crash here.


================
Comment at: llvm/tools/llvm-exegesis/lib/SubprocessMemory.cpp:138
+SubprocessMemory::~SubprocessMemory() {
+  report_fatal_error("~SubprocessMemory is only supported on Linux");
+}
----------------
This is not really needed.


================
Comment at: llvm/tools/llvm-exegesis/lib/SubprocessMemory.h:33
+  Error addMemoryDefinition(
+      std::unordered_map<std::string, MemoryValue> MemoryDefinitions,
+      pid_t ProcessID);
----------------
Please document these. What do the strings represent ?


================
Comment at: llvm/unittests/tools/llvm-exegesis/X86/SubprocessMemoryTest.cpp:31
+             std::unordered_map<std::string, MemoryValue> MemoryDefinitions) {
+    EXPECT_FALSE(SM.initializeSubprocessMemory(0));
+    EXPECT_FALSE(SM.addMemoryDefinition(MemoryDefinitions, 0));
----------------
Please extract this to a constant: `constexpr int kMainProcessPid = 0;`, and use it throughout the code for readability.


================
Comment at: llvm/unittests/tools/llvm-exegesis/X86/SubprocessMemoryTest.cpp:35
+
+  void checkSharedMemoryDefinition(std::string DefinitionName,
+                                   size_t DefinitionSize,
----------------
[nit] `const std::string&`


================
Comment at: llvm/unittests/tools/llvm-exegesis/X86/SubprocessMemoryTest.cpp:51
+TEST_F(SubprocessMemoryTest, OneDefinition) {
+  SubprocessMemory SM;
+  std::unordered_map<std::string, MemoryValue> MemoryDefinitions{
----------------
This can be moved to the test fixture (here and below).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D151022