[PATCH] D151021: [llvm-exegesis] Introduce Subprocess Executor Mode

[Clement Courbet via Phabricator via llvm-commits]

courbet added inline comments.


================
Comment at: llvm/test/tools/llvm-exegesis/X86/latency/subprocess-segfault.s:7
+
+# LLVM-EXEGESIS-DEFREG RBX 8192
+movq (%rbx), %rax
----------------
is this guaranteed to segfault ? maybe use `0` ?


================
Comment at: llvm/tools/llvm-exegesis/lib/BenchmarkRunner.cpp:145
+#ifdef __linux__
+class SubProcessFunctionExecutorImpl
+    : public BenchmarkRunner::FunctionExecutor {
----------------
Please add a simple class description.


================
Comment at: llvm/tools/llvm-exegesis/lib/BenchmarkRunner.cpp:189
+      llvm::sys::unregisterHandlers();
+      prepareAndRunBenchmark(PipeFiles[0], Key);
+    }
----------------
Add a comment here: `// Note: terminates the child process.`.


================
Comment at: llvm/tools/llvm-exegesis/lib/BenchmarkRunner.cpp:222
+    if(WIFEXITED(ChildStatus)) {
+      if (WEXITSTATUS(ChildStatus) == 0) {
+          // The child exited succesfully, read counter values and return
----------------
[nit] create a temporary variable for this to avoid repetition below ? These macros are not exactly self-explanatory.


================
Comment at: llvm/tools/llvm-exegesis/lib/BenchmarkRunner.cpp:247
+
+  void prepareAndRunBenchmark(int Pipe, const BenchmarkKey &Key) const {
+    // The following occurs within the benchmarking subprocess
----------------
`[[noreturn]]` ?


================
Comment at: llvm/tools/llvm-exegesis/lib/BenchmarkRunner.cpp:278
+    Error PossibleBenchmarkError =
+        createProcessAndRunBenchmark(CounterName, Value);
+
----------------
[nit] `createSubProcessAndRunBenchmark` for consistency ?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D151021