From 6d0afd5b1c4564255e76b1caf06b0ad51b329372 Mon Sep 17 00:00:00 2001 From: Aiden Grossman Date: Sun, 19 Nov 2023 23:04:59 -0800 Subject: [PATCH] [llvm-exegesis] Refactor ExecutableFunction to use a named constructor 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. --- llvm/tools/llvm-exegesis/lib/Assembler.cpp | 26 +++++---- llvm/tools/llvm-exegesis/lib/Assembler.h | 17 ++++-- .../llvm-exegesis/lib/BenchmarkRunner.cpp | 57 +++++++++++++++---- .../llvm-exegesis/Common/AssemblerUtils.h | 10 +++- 4 files changed, 81 insertions(+), 29 deletions(-) diff --git a/llvm/tools/llvm-exegesis/lib/Assembler.cpp b/llvm/tools/llvm-exegesis/lib/Assembler.cpp index 167fb6373377c..9ff33258e965f 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::create( std::unique_ptr TM, - object::OwningBinary &&ObjectFileHolder) - : Context(std::make_unique()) { + object::OwningBinary &&ObjectFileHolder) { assert(ObjectFileHolder.getBinary() && "cannot create object file"); + std::unique_ptr Ctx = std::make_unique(); // 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 EE( + EngineBuilder(createModule(Ctx, TM->createDataLayout())) .setErrorStr(&Error) .setMCPU(TM->getTargetCPU()) .setEngineKind(EngineKind::JIT) .setMCJITMemoryManager( std::make_unique(&CodeSize)) .create(TM.release())); - if (!ExecEngine) - report_fatal_error(Twine(Error)); + if (!EE) + return make_error(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(FunctionAddress), CodeSize); + return ExecutableFunction(std::move(Ctx), std::move(EE), FBytes); } +ExecutableFunction::ExecutableFunction(std::unique_ptr Ctx, + std::unique_ptr EE, + StringRef FB) + : FunctionBytes(FB), Context(std::move(Ctx)), ExecEngine(std::move(EE)) {} + Error getBenchmarkFunctionBytes(const StringRef InputData, std::vector &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 7c2a002967af7..5f1bf8cdfb7ad 100644 --- a/llvm/tools/llvm-exegesis/lib/Assembler.h +++ b/llvm/tools/llvm-exegesis/lib/Assembler.h @@ -106,10 +106,11 @@ object::OwningBinary getObjectFromFile(StringRef Filename); // Consumes an ObjectFile containing a `void foo(char*)` function and make it // executable. -struct ExecutableFunction { - explicit ExecutableFunction( - std::unique_ptr TM, - object::OwningBinary &&ObjectFileHolder); +class ExecutableFunction { +public: + static Expected + create(std::unique_ptr TM, + object::OwningBinary &&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 Ctx, + std::unique_ptr EE, + StringRef FunctionBytes); + std::unique_ptr Context; std::unique_ptr 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 7ec24eb2f866f..bd116ccb1df48 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> + create(const LLVMState &State, object::OwningBinary Obj, + BenchmarkRunner::ScratchSpace *Scratch) { + Expected EF = + ExecutableFunction::create(State.createTargetMachine(), std::move(Obj)); + + if (!EF) + return EF.takeError(); + + return std::unique_ptr( + new InProcessFunctionExecutorImpl(State, std::move(*EF), Scratch)); + } + +private: InProcessFunctionExecutorImpl(const LLVMState &State, - object::OwningBinary 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 &NewValues, llvm::SmallVector *Result) { @@ -161,13 +173,24 @@ class InProcessFunctionExecutorImpl : public BenchmarkRunner::FunctionExecutor { class SubProcessFunctionExecutorImpl : public BenchmarkRunner::FunctionExecutor { public: + static Expected> + create(const LLVMState &State, object::OwningBinary Obj, + const BenchmarkKey &Key) { + Expected EF = + ExecutableFunction::create(State.createTargetMachine(), std::move(Obj)); + if (!EF) + return EF.takeError(); + + return std::unique_ptr( + new SubProcessFunctionExecutorImpl(State, std::move(*EF), Key)); + } + +private: SubProcessFunctionExecutorImpl(const LLVMState &State, - object::OwningBinary 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 ObjectFile, const BenchmarkKey &Key) const { switch (ExecutionMode) { - case ExecutionModeE::InProcess: - return std::make_unique( + 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( + auto SubProcessExecutorOrErr = SubProcessFunctionExecutorImpl::create( State, std::move(ObjectFile), Key); + if (!SubProcessExecutorOrErr) + return SubProcessExecutorOrErr.takeError(); + + return std::move(*SubProcessExecutorOrErr); #else return make_error( "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 02706ca4c64cd..2804a6e69e824 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 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;