[llvm] r312429 - [ORC] Add an Error return to the JITCompileCallbackManager::grow method.

David Blaikie via llvm-commits llvm-commits at lists.llvm.org
Mon Sep 4 09:22:24 PDT 2017


On Sat, Sep 2, 2017 at 5:51 PM Lang Hames via llvm-commits <
llvm-commits at lists.llvm.org> wrote:

> Author: lhames
> Date: Sat Sep  2 17:50:42 2017
> New Revision: 312429
>
> URL: http://llvm.org/viewvc/llvm-project?rev=312429&view=rev
> Log:
> [ORC] Add an Error return to the JITCompileCallbackManager::grow method.
>
> Calling grow may result in an error if, for example, this is a callback
> manager for a remote target. We need to be able to return this error to the
> callee.
>
>
> Modified:
>
> llvm/trunk/examples/Kaleidoscope/BuildingAJIT/Chapter4/KaleidoscopeJIT.h
>
> llvm/trunk/examples/Kaleidoscope/BuildingAJIT/Chapter5/KaleidoscopeJIT.h
>     llvm/trunk/include/llvm/ExecutionEngine/Orc/CompileOnDemandLayer.h
>     llvm/trunk/include/llvm/ExecutionEngine/Orc/IndirectionUtils.h
>     llvm/trunk/include/llvm/ExecutionEngine/Orc/OrcRemoteTargetClient.h
>     llvm/trunk/lib/ExecutionEngine/Orc/OrcCBindingsStack.h
>     llvm/trunk/unittests/ExecutionEngine/Orc/CompileOnDemandLayerTest.cpp
>
> Modified:
> llvm/trunk/examples/Kaleidoscope/BuildingAJIT/Chapter4/KaleidoscopeJIT.h
> URL:
> http://llvm.org/viewvc/llvm-project/llvm/trunk/examples/Kaleidoscope/BuildingAJIT/Chapter4/KaleidoscopeJIT.h?rev=312429&r1=312428&r2=312429&view=diff
>
> ==============================================================================
> ---
> llvm/trunk/examples/Kaleidoscope/BuildingAJIT/Chapter4/KaleidoscopeJIT.h
> (original)
> +++
> llvm/trunk/examples/Kaleidoscope/BuildingAJIT/Chapter4/KaleidoscopeJIT.h
> Sat Sep  2 17:50:42 2017
> @@ -135,7 +135,7 @@ public:
>    Error addFunctionAST(std::unique_ptr<FunctionAST> FnAST) {
>      // Create a CompileCallback - this is the re-entry point into the
> compiler
>      // for functions that haven't been compiled yet.
> -    auto CCInfo = CompileCallbackMgr->getCompileCallback();
> +    auto CCInfo = cantFail(CompileCallbackMgr->getCompileCallback());
>
>      // Create an indirect stub. This serves as the functions "canonical
>      // definition" - an unchanging (constant address) entry point to the
>
> Modified:
> llvm/trunk/examples/Kaleidoscope/BuildingAJIT/Chapter5/KaleidoscopeJIT.h
> URL:
> http://llvm.org/viewvc/llvm-project/llvm/trunk/examples/Kaleidoscope/BuildingAJIT/Chapter5/KaleidoscopeJIT.h?rev=312429&r1=312428&r2=312429&view=diff
>
> ==============================================================================
> ---
> llvm/trunk/examples/Kaleidoscope/BuildingAJIT/Chapter5/KaleidoscopeJIT.h
> (original)
> +++
> llvm/trunk/examples/Kaleidoscope/BuildingAJIT/Chapter5/KaleidoscopeJIT.h
> Sat Sep  2 17:50:42 2017
> @@ -164,7 +164,7 @@ public:
>    Error addFunctionAST(std::unique_ptr<FunctionAST> FnAST) {
>      // Create a CompileCallback - this is the re-entry point into the
> compiler
>      // for functions that haven't been compiled yet.
> -    auto CCInfo = CompileCallbackMgr->getCompileCallback();
> +    auto CCInfo = cantFail(CompileCallbackMgr->getCompileCallback());
>
>      // Create an indirect stub. This serves as the functions "canonical
>      // definition" - an unchanging (constant address) entry point to the
>
> Modified:
> llvm/trunk/include/llvm/ExecutionEngine/Orc/CompileOnDemandLayer.h
> URL:
> http://llvm.org/viewvc/llvm-project/llvm/trunk/include/llvm/ExecutionEngine/Orc/CompileOnDemandLayer.h?rev=312429&r1=312428&r2=312429&view=diff
>
> ==============================================================================
> --- llvm/trunk/include/llvm/ExecutionEngine/Orc/CompileOnDemandLayer.h
> (original)
> +++ llvm/trunk/include/llvm/ExecutionEngine/Orc/CompileOnDemandLayer.h Sat
> Sep  2 17:50:42 2017
> @@ -349,19 +349,22 @@ private:
>          // Create a callback, associate it with the stub for the function,
>          // and set the compile action to compile the partition containing
> the
>          // function.
> -        auto CCInfo = CompileCallbackMgr.getCompileCallback();
> -        StubInits[MangledName] =
> -          std::make_pair(CCInfo.getAddress(),
> -                         JITSymbolFlags::fromGlobalValue(F));
> -        CCInfo.setCompileAction([this, &LD, LMId, &F]() ->
> JITTargetAddress {
> -            if (auto FnImplAddrOrErr = this->extractAndCompile(LD, LMId,
> F))
> -              return *FnImplAddrOrErr;
> -            else {
> -              // FIXME: Report error, return to 'abort' or something
> similar.
> -              consumeError(FnImplAddrOrErr.takeError());
> -              return 0;
> -            }
> -          });
> +        if (auto CCInfoOrErr = CompileCallbackMgr.getCompileCallback()) {
>

As much as I like the scoping of declaring things in conditions, the
indentation this causes makes this code a bit harder to follow compared to:

  auto CCInfoOrErr = ...
  if (!CCInfoOrErr)
    return CCInfoOrErr.takeError();
  auto &CCInfo = *CCInfoOrErr;
  ...

& then similarly in the FnIMplAddrOrErr codepath and TrampolineAddrOrErr
below.

Just a thought - up to you though.


> +          auto &CCInfo = *CCInfoOrErr;
> +          StubInits[MangledName] =
> +            std::make_pair(CCInfo.getAddress(),
> +                           JITSymbolFlags::fromGlobalValue(F));
> +          CCInfo.setCompileAction([this, &LD, LMId, &F]() ->
> JITTargetAddress {
> +              if (auto FnImplAddrOrErr = this->extractAndCompile(LD,
> LMId, F))
> +                return *FnImplAddrOrErr;
> +              else {
> +                // FIXME: Report error, return to 'abort' or something
> similar.
> +                consumeError(FnImplAddrOrErr.takeError());
> +                return 0;
> +              }
> +            });
> +        } else
> +          return CCInfoOrErr.takeError();
>        }
>
>        if (auto Err = LD.StubsMgr->createStubs(StubInits))
>
> Modified: llvm/trunk/include/llvm/ExecutionEngine/Orc/IndirectionUtils.h
> URL:
> http://llvm.org/viewvc/llvm-project/llvm/trunk/include/llvm/ExecutionEngine/Orc/IndirectionUtils.h?rev=312429&r1=312428&r2=312429&view=diff
>
> ==============================================================================
> --- llvm/trunk/include/llvm/ExecutionEngine/Orc/IndirectionUtils.h
> (original)
> +++ llvm/trunk/include/llvm/ExecutionEngine/Orc/IndirectionUtils.h Sat
> Sep  2 17:50:42 2017
> @@ -105,10 +105,13 @@ public:
>    }
>
>    /// @brief Reserve a compile callback.
> -  CompileCallbackInfo getCompileCallback() {
> -    JITTargetAddress TrampolineAddr = getAvailableTrampolineAddr();
> -    auto &Compile = this->ActiveTrampolines[TrampolineAddr];
> -    return CompileCallbackInfo(TrampolineAddr, Compile);
> +  Expected<CompileCallbackInfo> getCompileCallback() {
> +    if (auto TrampolineAddrOrErr = getAvailableTrampolineAddr()) {
> +      const auto &TrampolineAddr = *TrampolineAddrOrErr;
> +      auto &Compile = this->ActiveTrampolines[TrampolineAddr];
> +      return CompileCallbackInfo(TrampolineAddr, Compile);
> +    } else
> +      return TrampolineAddrOrErr.takeError();
>    }
>
>    /// @brief Get a CompileCallbackInfo for an existing callback.
> @@ -138,9 +141,10 @@ protected:
>    std::vector<JITTargetAddress> AvailableTrampolines;
>
>  private:
> -  JITTargetAddress getAvailableTrampolineAddr() {
> +  Expected<JITTargetAddress> getAvailableTrampolineAddr() {
>      if (this->AvailableTrampolines.empty())
> -      grow();
> +      if (auto Err = grow())
> +        return std::move(Err);
>      assert(!this->AvailableTrampolines.empty() &&
>             "Failed to grow available trampolines.");
>      JITTargetAddress TrampolineAddr = this->AvailableTrampolines.back();
> @@ -149,7 +153,7 @@ private:
>    }
>
>    // Create new trampolines - to be implemented in subclasses.
> -  virtual void grow() = 0;
> +  virtual Error grow() = 0;
>
>    virtual void anchor();
>  };
> @@ -188,7 +192,7 @@ private:
>              reinterpret_cast<uintptr_t>(TrampolineId)));
>    }
>
> -  void grow() override {
> +  Error grow() override {
>      assert(this->AvailableTrampolines.empty() && "Growing prematurely?");
>
>      std::error_code EC;
> @@ -196,7 +200,8 @@ private:
>          sys::OwningMemoryBlock(sys::Memory::allocateMappedMemory(
>              sys::Process::getPageSize(), nullptr,
>              sys::Memory::MF_READ | sys::Memory::MF_WRITE, EC));
> -    assert(!EC && "Failed to allocate trampoline block");
> +    if (EC)
> +      return errorCodeToError(EC);
>
>      unsigned NumTrampolines =
>          (sys::Process::getPageSize() - TargetT::PointerSize) /
> @@ -211,12 +216,13 @@ private:
>            static_cast<JITTargetAddress>(reinterpret_cast<uintptr_t>(
>                TrampolineMem + (I * TargetT::TrampolineSize))));
>
> -    EC =
> sys::Memory::protectMappedMemory(TrampolineBlock.getMemoryBlock(),
> -                                          sys::Memory::MF_READ |
> -                                              sys::Memory::MF_EXEC);
> -    assert(!EC && "Failed to mprotect trampoline block");
> +    if (auto EC = sys::Memory::protectMappedMemory(
> +                    TrampolineBlock.getMemoryBlock(),
> +                    sys::Memory::MF_READ | sys::Memory::MF_EXEC))
> +      return errorCodeToError(EC);
>
>      TrampolineBlocks.push_back(std::move(TrampolineBlock));
> +    return Error::success();
>    }
>
>    sys::OwningMemoryBlock ResolverBlock;
>
> Modified:
> llvm/trunk/include/llvm/ExecutionEngine/Orc/OrcRemoteTargetClient.h
> URL:
> http://llvm.org/viewvc/llvm-project/llvm/trunk/include/llvm/ExecutionEngine/Orc/OrcRemoteTargetClient.h?rev=312429&r1=312428&r2=312429&view=diff
>
> ==============================================================================
> --- llvm/trunk/include/llvm/ExecutionEngine/Orc/OrcRemoteTargetClient.h
> (original)
> +++ llvm/trunk/include/llvm/ExecutionEngine/Orc/OrcRemoteTargetClient.h
> Sat Sep  2 17:50:42 2017
> @@ -543,19 +543,19 @@ public:
>          : JITCompileCallbackManager(ErrorHandlerAddress), Remote(Remote)
> {}
>
>    private:
> -    void grow() override {
> +    Error grow() override {
>        JITTargetAddress BlockAddr = 0;
>        uint32_t NumTrampolines = 0;
>        if (auto TrampolineInfoOrErr = Remote.emitTrampolineBlock())
>          std::tie(BlockAddr, NumTrampolines) = *TrampolineInfoOrErr;
> -      else {
> -        // FIXME: Return error.
> -        llvm_unreachable("Failed to create trampolines");
> -      }
> +      else
> +        return TrampolineInfoOrErr.takeError();
>
>        uint32_t TrampolineSize = Remote.getTrampolineSize();
>        for (unsigned I = 0; I < NumTrampolines; ++I)
>          this->AvailableTrampolines.push_back(BlockAddr + (I *
> TrampolineSize));
> +
> +      return Error::success();
>      }
>
>      OrcRemoteTargetClient &Remote;
>
> Modified: llvm/trunk/lib/ExecutionEngine/Orc/OrcCBindingsStack.h
> URL:
> http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/ExecutionEngine/Orc/OrcCBindingsStack.h?rev=312429&r1=312428&r2=312429&view=diff
>
> ==============================================================================
> --- llvm/trunk/lib/ExecutionEngine/Orc/OrcCBindingsStack.h (original)
> +++ llvm/trunk/lib/ExecutionEngine/Orc/OrcCBindingsStack.h Sat Sep  2
> 17:50:42 2017
> @@ -145,12 +145,15 @@ public:
>    createLazyCompileCallback(JITTargetAddress &RetAddr,
>                              LLVMOrcLazyCompileCallbackFn Callback,
>                              void *CallbackCtx) {
> -    auto CCInfo = CCMgr->getCompileCallback();
> -    CCInfo.setCompileAction([=]() -> JITTargetAddress {
> -      return Callback(wrap(this), CallbackCtx);
> -    });
> -    RetAddr = CCInfo.getAddress();
> -    return LLVMOrcErrSuccess;
> +    if (auto CCInfoOrErr = CCMgr->getCompileCallback()) {
> +      auto &CCInfo = *CCInfoOrErr;
> +      CCInfo.setCompileAction([=]() -> JITTargetAddress {
> +          return Callback(wrap(this), CallbackCtx);
> +        });
> +      RetAddr = CCInfo.getAddress();
> +      return LLVMOrcErrSuccess;
> +    } else
> +      return mapError(CCInfoOrErr.takeError());
>    }
>
>    LLVMOrcErrorCode createIndirectStub(StringRef StubName,
>
> Modified:
> llvm/trunk/unittests/ExecutionEngine/Orc/CompileOnDemandLayerTest.cpp
> URL:
> http://llvm.org/viewvc/llvm-project/llvm/trunk/unittests/ExecutionEngine/Orc/CompileOnDemandLayerTest.cpp?rev=312429&r1=312428&r2=312429&view=diff
>
> ==============================================================================
> --- llvm/trunk/unittests/ExecutionEngine/Orc/CompileOnDemandLayerTest.cpp
> (original)
> +++ llvm/trunk/unittests/ExecutionEngine/Orc/CompileOnDemandLayerTest.cpp
> Sat Sep  2 17:50:42 2017
> @@ -21,7 +21,7 @@ public:
>    DummyCallbackManager() : JITCompileCallbackManager(0) {}
>
>  public:
> -  void grow() override { llvm_unreachable("not implemented"); }
> +  Error grow() override { llvm_unreachable("not implemented"); }
>  };
>
>  class DummyStubsManager : public orc::IndirectStubsManager {
>
>
> _______________________________________________
> llvm-commits mailing list
> llvm-commits at lists.llvm.org
> http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20170904/fe19b6b5/attachment.html>


More information about the llvm-commits mailing list