[PATCH] D144276: [ORC] Introduce SetUpExecutorNativePlatform utility.

[Aman LaChapelle via Phabricator via llvm-commits]

bzcheeseman added inline comments.


================
Comment at: llvm/include/llvm/ExecutionEngine/Orc/LLJIT.h:456
+/// native target for the executor.
+class SetUpExecutorNativePlatform {
+public:
----------------
lhames wrote:
> sunho wrote:
> > Something worth adding to this interface for COFF side of view is add constructor that receive vcruntime path. Without manually shipping them program will not run for people that do not have vc toolchain installed in their computer. Even better instead of path receive memorybuffer so that ORC users can embed vcruntime libs.
> > 
> > Maybe this class is not right place to add though as its supposed to be generic it seems but we do have this kind of use case.
> I meant to reply earlier: this is a great idea.
> 
> @bzcheeseman this is the comment that I wanted to address before I landed the patch, I've just been too busy.
> 
> I think we want this class to contain optional paths for all necessary runtimes. E.g. if we added
> 
> ```
>   SetUpExecutorNativePlatform &addVCRuntimePath(std::string VCRuntimePath) {
>     this->VCRuntimePath = std::move(VCRuntimePath);
>     return *this;
>   }
> ```
> and a `VCRuntimePath` member, then in `lli` we could write:
> 
> ```
> Builder.setPlatformSetUp(
>   orc::SetUpExecutorNativePlatform(
>     OrcRuntime, std::move(PlatformPrepare))
>   .addVCRuntimePath("/path/to/VCRuntime"));
> ```
> 
> `SetUpExecutorNativePlatform` could take the union of info for all platforms, then check that it has the paths that it needs to instantiate the required platform at runtime, or error out with an explanation, e.g. "Executor is running Windows but VC runtime path not specified".
I'm happy to do this as a follow up patch if you want, it seems pretty simple.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D144276