[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:50
+  // The size of the value in bytes.
+  size_t Size;
+  // The index of the memory value.
----------------
`SizeBytes` then ?


================
Comment at: llvm/tools/llvm-exegesis/lib/BenchmarkResult.h:52
+  // The index of the memory value.
+  size_t Number;
+};
----------------
`Idx` ?


================
Comment at: llvm/tools/llvm-exegesis/lib/SubprocessMemory.h:46-47
+  // Arguments:
+  // MemoryDefinitions - A map from memory values
+  // names to Memoryvalues. ParentPID - The ID of the process that setup the
+  // memory definitions. CounterFileDescriptor - The file descriptor for the
----------------
reflow


================
Comment at: llvm/tools/llvm-exegesis/lib/SubprocessMemory.h:47
+  // MemoryDefinitions - A map from memory values
+  // names to Memoryvalues. ParentPID - The ID of the process that setup the
+  // memory definitions. CounterFileDescriptor - The file descriptor for the
----------------
reflow


================
Comment at: llvm/tools/llvm-exegesis/lib/SubprocessMemory.h:48
+  // names to Memoryvalues. ParentPID - The ID of the process that setup the
+  // memory definitions. CounterFileDescriptor - The file descriptor for the
+  // performance counter that will be placed in the auxiliary memory section.
----------------
reflow


================
Comment at: llvm/unittests/tools/llvm-exegesis/X86/SubprocessMemoryTest.cpp:54
+TEST_F(SubprocessMemoryTest, OneDefinition) {
+  SubprocessMemory SM = testCommon({{"test1", {APInt(8, 0xff), 4096, 0}}});
+  checkSharedMemoryDefinition("/0memdef0", 4096, {0xff});
----------------
The typical gtest style for this would be for `SM` to be a member the test fixture:

```
class SubprocessMemoryTest : public X86TestBase {
 protected:
  void setupSuprocessMemory(...) {
    EXPECT_FALSE(SM.initializeSubprocessMemory(MainProcessPID));
    EXPECT_FALSE(SM.addMemoryDefinition(MemoryDefinitions, MainProcessPID));
  }

  SubprocessMemory SM;
};


TEST_F(SubprocessMemoryTest, OneDefinition) {
  // note: SM is available here if you need it, we're in the context of the test fixture.
  setupSuprocessMemory({{"test1", {APInt(8, 0xff), 4096, 0}}});
  checkSharedMemoryDefinition("/0memdef2", 4096, {0xcc});
}
```



================
Comment at: llvm/unittests/tools/llvm-exegesis/X86/SubprocessMemoryTest.cpp:83
+  SubprocessMemory SM =
+      testCommon({{"test1", {APInt(48, 0xaabbccddeeff), 4096, 0}}});
+  std::vector<uint8_t> Test1Expected(512, 0);
----------------
If you want to support fractional sizes should add a test for it.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D151022