[llvm-exegesis] Refactor ExecutableFunction to use a named constructor

[boomanaiden154]

This patch refactors ExecutableFunction to use a named constructor pattern, namely adding the create function, so that errors occurring during the creation of an ExecutableFunction can be propogated back up rather than having to deal with them in report_fatal_error.

[llvmbot]

@llvm/pr-subscribers-tools-llvm-exegesis

Author: Aiden Grossman (boomanaiden154)

Changes

This patch refactors ExecutableFunction to use a named constructor pattern, namely adding the create function, so that errors occurring during the creation of an ExecutableFunction can be propogated back up rather than having to deal with them in report_fatal_error.

---

Full diff: https://github.com/llvm/llvm-project/pull/72837.diff

4 Files Affected:

• (modified) llvm/tools/llvm-exegesis/lib/Assembler.cpp (+16-10)
• (modified) llvm/tools/llvm-exegesis/lib/Assembler.h (+12-5)
• (modified) llvm/tools/llvm-exegesis/lib/BenchmarkRunner.cpp (+45-12)
• (modified) llvm/unittests/tools/llvm-exegesis/Common/AssemblerUtils.h (+8-2)

diff --git a/llvm/tools/llvm-exegesis/lib/Assembler.cpp b/llvm/tools/llvm-exegesis/lib/Assembler.cpp
index 167fb6373377c28..9ff33258e965f7e 100644
--- a/llvm/tools/llvm-exegesis/lib/Assembler.cpp
+++ b/llvm/tools/llvm-exegesis/lib/Assembler.cpp
@@ -347,38 +347,44 @@ class TrackingSectionMemoryManager : public SectionMemoryManager {
 
 } // namespace
 
-ExecutableFunction::ExecutableFunction(
+Expected<ExecutableFunction> ExecutableFunction::create(
     std::unique_ptr<LLVMTargetMachine> TM,
-    object::OwningBinary<object::ObjectFile> &&ObjectFileHolder)
-    : Context(std::make_unique<LLVMContext>()) {
+    object::OwningBinary<object::ObjectFile> &&ObjectFileHolder) {
   assert(ObjectFileHolder.getBinary() && "cannot create object file");
+  std::unique_ptr<LLVMContext> Ctx = std::make_unique<LLVMContext>();
   // Initializing the execution engine.
   // We need to use the JIT EngineKind to be able to add an object file.
   LLVMLinkInMCJIT();
   uintptr_t CodeSize = 0;
   std::string Error;
-  ExecEngine.reset(
-      EngineBuilder(createModule(Context, TM->createDataLayout()))
+  std::unique_ptr<ExecutionEngine> EE(
+      EngineBuilder(createModule(Ctx, TM->createDataLayout()))
           .setErrorStr(&Error)
           .setMCPU(TM->getTargetCPU())
           .setEngineKind(EngineKind::JIT)
           .setMCJITMemoryManager(
               std::make_unique<TrackingSectionMemoryManager>(&CodeSize))
           .create(TM.release()));
-  if (!ExecEngine)
-    report_fatal_error(Twine(Error));
+  if (!EE)
+    return make_error<StringError>(Twine(Error), inconvertibleErrorCode());
   // Adding the generated object file containing the assembled function.
   // The ExecutionEngine makes sure the object file is copied into an
   // executable page.
-  ExecEngine->addObjectFile(std::move(ObjectFileHolder));
+  EE->addObjectFile(std::move(ObjectFileHolder));
   // Fetching function bytes.
-  const uint64_t FunctionAddress = ExecEngine->getFunctionAddress(FunctionID);
+  const uint64_t FunctionAddress = EE->getFunctionAddress(FunctionID);
   assert(isAligned(kFunctionAlignment, FunctionAddress) &&
          "function is not properly aligned");
-  FunctionBytes =
+  StringRef FBytes =
       StringRef(reinterpret_cast<const char *>(FunctionAddress), CodeSize);
+  return ExecutableFunction(std::move(Ctx), std::move(EE), FBytes);
 }
 
+ExecutableFunction::ExecutableFunction(std::unique_ptr<LLVMContext> Ctx,
+                                       std::unique_ptr<ExecutionEngine> EE,
+                                       StringRef FB)
+    : FunctionBytes(FB), Context(std::move(Ctx)), ExecEngine(std::move(EE)) {}
+
 Error getBenchmarkFunctionBytes(const StringRef InputData,
                                 std::vector<uint8_t> &Bytes) {
   const auto Holder = getObjectFromBuffer(InputData);
diff --git a/llvm/tools/llvm-exegesis/lib/Assembler.h b/llvm/tools/llvm-exegesis/lib/Assembler.h
index 7c2a002967af720..5f1bf8cdfb7ad6c 100644
--- a/llvm/tools/llvm-exegesis/lib/Assembler.h
+++ b/llvm/tools/llvm-exegesis/lib/Assembler.h
@@ -106,10 +106,11 @@ object::OwningBinary<object::ObjectFile> getObjectFromFile(StringRef Filename);
 
 // Consumes an ObjectFile containing a `void foo(char*)` function and make it
 // executable.
-struct ExecutableFunction {
-  explicit ExecutableFunction(
-      std::unique_ptr<LLVMTargetMachine> TM,
-      object::OwningBinary<object::ObjectFile> &&ObjectFileHolder);
+class ExecutableFunction {
+public:
+  static Expected<ExecutableFunction>
+  create(std::unique_ptr<LLVMTargetMachine> TM,
+         object::OwningBinary<object::ObjectFile> &&ObjectFileHolder);
 
   // Retrieves the function as an array of bytes.
   StringRef getFunctionBytes() const { return FunctionBytes; }
@@ -119,9 +120,15 @@ struct ExecutableFunction {
     ((void (*)(char *))(intptr_t)FunctionBytes.data())(Memory);
   }
 
+  StringRef FunctionBytes;
+
+private:
+  ExecutableFunction(std::unique_ptr<LLVMContext> Ctx,
+                     std::unique_ptr<ExecutionEngine> EE,
+                     StringRef FunctionBytes);
+
   std::unique_ptr<LLVMContext> Context;
   std::unique_ptr<ExecutionEngine> ExecEngine;
-  StringRef FunctionBytes;
 };
 
 // Copies benchmark function's bytes from benchmark object.
diff --git a/llvm/tools/llvm-exegesis/lib/BenchmarkRunner.cpp b/llvm/tools/llvm-exegesis/lib/BenchmarkRunner.cpp
index 7ec24eb2f866f86..bd116ccb1df4866 100644
--- a/llvm/tools/llvm-exegesis/lib/BenchmarkRunner.cpp
+++ b/llvm/tools/llvm-exegesis/lib/BenchmarkRunner.cpp
@@ -89,13 +89,25 @@ BenchmarkRunner::FunctionExecutor::runAndSample(const char *Counters) const {
 namespace {
 class InProcessFunctionExecutorImpl : public BenchmarkRunner::FunctionExecutor {
 public:
+  static Expected<std::unique_ptr<InProcessFunctionExecutorImpl>>
+  create(const LLVMState &State, object::OwningBinary<object::ObjectFile> Obj,
+         BenchmarkRunner::ScratchSpace *Scratch) {
+    Expected<ExecutableFunction> EF =
+        ExecutableFunction::create(State.createTargetMachine(), std::move(Obj));
+
+    if (!EF)
+      return EF.takeError();
+
+    return std::unique_ptr<InProcessFunctionExecutorImpl>(
+        new InProcessFunctionExecutorImpl(State, std::move(*EF), Scratch));
+  }
+
+private:
   InProcessFunctionExecutorImpl(const LLVMState &State,
-                                object::OwningBinary<object::ObjectFile> Obj,
+                                ExecutableFunction Function,
                                 BenchmarkRunner::ScratchSpace *Scratch)
-      : State(State), Function(State.createTargetMachine(), std::move(Obj)),
-        Scratch(Scratch) {}
+      : State(State), Function(std::move(Function)), Scratch(Scratch) {}
 
-private:
   static void
   accumulateCounterValues(const llvm::SmallVector<int64_t, 4> &NewValues,
                           llvm::SmallVector<int64_t, 4> *Result) {
@@ -161,13 +173,24 @@ class InProcessFunctionExecutorImpl : public BenchmarkRunner::FunctionExecutor {
 class SubProcessFunctionExecutorImpl
     : public BenchmarkRunner::FunctionExecutor {
 public:
+  static Expected<std::unique_ptr<SubProcessFunctionExecutorImpl>>
+  create(const LLVMState &State, object::OwningBinary<object::ObjectFile> Obj,
+         const BenchmarkKey &Key) {
+    Expected<ExecutableFunction> EF =
+        ExecutableFunction::create(State.createTargetMachine(), std::move(Obj));
+    if (!EF)
+      return EF.takeError();
+
+    return std::unique_ptr<SubProcessFunctionExecutorImpl>(
+        new SubProcessFunctionExecutorImpl(State, std::move(*EF), Key));
+  }
+
+private:
   SubProcessFunctionExecutorImpl(const LLVMState &State,
-                                 object::OwningBinary<object::ObjectFile> Obj,
+                                 ExecutableFunction Function,
                                  const BenchmarkKey &Key)
-      : State(State), Function(State.createTargetMachine(), std::move(Obj)),
-        Key(Key) {}
+      : State(State), Function(std::move(Function)), Key(Key) {}
 
-private:
   enum ChildProcessExitCodeE {
     CounterFDReadFailed = 1,
     RSeqDisableFailed,
@@ -490,17 +513,27 @@ BenchmarkRunner::createFunctionExecutor(
     object::OwningBinary<object::ObjectFile> ObjectFile,
     const BenchmarkKey &Key) const {
   switch (ExecutionMode) {
-  case ExecutionModeE::InProcess:
-    return std::make_unique<InProcessFunctionExecutorImpl>(
+  case ExecutionModeE::InProcess: {
+    auto InProcessExecutorOrErr = InProcessFunctionExecutorImpl::create(
         State, std::move(ObjectFile), Scratch.get());
-  case ExecutionModeE::SubProcess:
+    if (!InProcessExecutorOrErr)
+      return InProcessExecutorOrErr.takeError();
+
+    return std::move(*InProcessExecutorOrErr);
+  }
+  case ExecutionModeE::SubProcess: {
 #ifdef __linux__
-    return std::make_unique<SubProcessFunctionExecutorImpl>(
+    auto SubProcessExecutorOrErr = SubProcessFunctionExecutorImpl::create(
         State, std::move(ObjectFile), Key);
+    if (!SubProcessExecutorOrErr)
+      return SubProcessExecutorOrErr.takeError();
+
+    return std::move(*SubProcessExecutorOrErr);
 #else
     return make_error<Failure>(
         "The subprocess execution mode is only supported on Linux");
 #endif
+  }
   }
   llvm_unreachable("ExecutionMode is outside expected range");
 }
diff --git a/llvm/unittests/tools/llvm-exegesis/Common/AssemblerUtils.h b/llvm/unittests/tools/llvm-exegesis/Common/AssemblerUtils.h
index 02706ca4c64cd2c..2804a6e69e824e4 100644
--- a/llvm/unittests/tools/llvm-exegesis/Common/AssemblerUtils.h
+++ b/llvm/unittests/tools/llvm-exegesis/Common/AssemblerUtils.h
@@ -20,6 +20,7 @@
 #include "llvm/MC/TargetRegistry.h"
 #include "llvm/Support/TargetSelect.h"
 #include "llvm/TargetParser/Host.h"
+#include "llvm/Testing/Support/Error.h"
 #include "gmock/gmock.h"
 #include "gtest/gtest.h"
 
@@ -82,8 +83,13 @@ class MachineFunctionGeneratorBaseTest : public ::testing::Test {
     EXPECT_FALSE(assembleToStream(*ET, createTargetMachine(), /*LiveIns=*/{},
                                   RegisterInitialValues, Fill, AsmStream, Key,
                                   false));
-    return ExecutableFunction(createTargetMachine(),
-                              getObjectFromBuffer(AsmStream.str()));
+    Expected<ExecutableFunction> ExecFunc = ExecutableFunction::create(
+        createTargetMachine(), getObjectFromBuffer(AsmStream.str()));
+
+    // We can't use ASSERT_THAT_EXPECTED here as it doesn't work inside of
+    // non-void functions.
+    EXPECT_TRUE(detail::TakeExpected(ExecFunc).Success());
+    return std::move(*ExecFunc);
   }
 
   const std::string TT;