[PATCH] D151019: [llvm-exegesis] Refactor FunctionExecutorImpl and create factory

Tue May 23 18:04:50 PDT 2023

aidengrossman added inline comments.

================
Comment at: llvm/tools/llvm-exegesis/lib/BenchmarkRunner.cpp:195

+  RC.ExecutionMode = ExecutionMode;
+
----------------
courbet wrote:
> A runnable configuration exists outside of how it's executed, it feels weird to have this here. Why isn't this a member of `BenchmarkRunner` ? 
Good point. I've switched it over to being a member of `BenchmarkRunner`.

================
Comment at: llvm/tools/llvm-exegesis/lib/BenchmarkRunner.cpp:204
+    object::OwningBinary<object::ObjectFile> ObjectFile,
+    const BenchmarkKey &Key) const {
+  if (ExecutionMode == ExecutionModeE::InProcess) {
----------------
courbet wrote:
> Why do we need `Key` ?
I'm using a `BenchmarkKey` in https://reviews.llvm.org/D151025 (and touching it in some previous patches in the stack) to pass around memory annotations, and the way I have it structured currently, I pass the key to the `FunctionExecutorImpl` for setup. I can move this to another diff/change this around if desired.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D151019