[clang-repl] Sink RemoteJITUtils into Interpreter class (NFC) #155140
Conversation
…into redirection
clang/tools/clang-repl/ClangRepl.cpp
Outdated
| if (::OffloadArch.empty()) { | ||
| ::OffloadArch = "sm_35"; | ||
| } | ||
| CB.SetOffloadArch(OffloadArch); | ||
| CB.SetOffloadArch(::OffloadArch); |
There was a problem hiding this comment.
Are these changes relevant to the current PR?
There was a problem hiding this comment.
yes. if not done, it gives following error:
error: reference to 'OffloadArch' is ambiguousThere was a problem hiding this comment.
Which compiler is this? Why it works upstream?
There was a problem hiding this comment.
(base) llvm-project-test % clang --version
Homebrew clang version 20.1.6
Target: arm64-apple-darwin24.5.0
Thread model: posix
InstalledDir: /opt/homebrew/Cellar/llvm/20.1.6/bin
Configuration file: /opt/homebrew/etc/clang/arm64-apple-darwin24.cfg'
There was a problem hiding this comment.
Not sure what's the problem but if that does not fail upstream please remove it. What are the other declarations that make OffloadArch ambiguous?
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
kr-2003
changed the title
clang/tools/clang-repl/ClangRepl.cpp
Outdated
| if (::OffloadArch.empty()) { | ||
| ::OffloadArch = "sm_35"; | ||
| } | ||
| CB.SetOffloadArch(OffloadArch); | ||
| CB.SetOffloadArch(::OffloadArch); |
There was a problem hiding this comment.
Not sure what's the problem but if that does not fail upstream please remove it. What are the other declarations that make OffloadArch ambiguous?
Sorry, it was mistake from my side. I included |
kr-2003
changed the title
vgvassilev
left a comment
vgvassilev
left a comment
There was a problem hiding this comment.
LGTM! Thank you for the patience.
| public: | ||
| struct JITConfig { | ||
| /// Indicates whether out-of-process JIT execution is enabled. | ||
| bool IsOutOfProcess = false; | ||
| /// Path to the out-of-process JIT executor. | ||
| std::string OOPExecutor = ""; | ||
| std::string OOPExecutorConnect = ""; | ||
| /// Indicates whether to use shared memory for communication. | ||
| bool UseSharedMemory = false; | ||
| /// Representing the slab allocation size for memory management in kb. | ||
| unsigned SlabAllocateSize = 0; | ||
| /// Path to the ORC runtime library. | ||
| std::string OrcRuntimePath = ""; | ||
| /// PID of the out-of-process JIT executor. | ||
| uint32_t ExecutorPID = 0; | ||
|
|
||
| JITConfig() | ||
| : IsOutOfProcess(false), OOPExecutor(""), OOPExecutorConnect(""), | ||
| UseSharedMemory(false), SlabAllocateSize(0), OrcRuntimePath(""), | ||
| ExecutorPID(0) {} | ||
| }; | ||
|
|
There was a problem hiding this comment.
From discussion with @vgvassilev it sounds like the motivation here was to defer creation of the LLJITBuilder until you can ask the driver for the runtime path?
This solves that problem, but eliminates the ability to configure the builder.
What if you added a new member and callback along the lines of:
std::optional<std::string> OrcRuntimePath;
llvm::unique_function<LLJITBuilder(&JITConfig)> MakeJITBuilder = makeDefaultJITBuilder;then in the setup path you'd have something like:
if (!Cfg.OrcRuntimePath) {
if (OrcRTPath = getOrcRuntimePath(TC))
CfgOrcRuntimePath = std::move(*OrcRTPath);
else
return OrcRTPath.takeError();
}
auto JITBuilder = MakeJITBuilder(Cfg);
if (auto JOrErr = JITBuilder.create())
J = std::move(*JOrErr);
else
return JOrErr.takeError();
...The extra indirection gives you the opportunity to ask the driver for the path (if one isn't provided) while still giving the client the opportunity to configure the LLJITBuilder.
3 participants
This is a refactoring PR. It sinks RemoteJITUtils into Interpreter and IncrementalExecutor classes.