Skip to content

Conversation

kr-2003
Copy link
Contributor

@kr-2003 kr-2003 commented Aug 24, 2025

This PR builds upon the changes introduced in #152562.

Introduces:

  1. Custom lambda function in launchExecutor.
  2. Fetching PID of the launched out-of-process(OOP) JIT executor.
  3. Dynamic fetching of liborc_rt path.

Comment on lines +334 to +337
if (::OffloadArch.empty()) {
::OffloadArch = "sm_35";
}
CB.SetOffloadArch(OffloadArch);
CB.SetOffloadArch(::OffloadArch);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are these changes relevant to the current PR?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes. if not done, it gives following error:

error: reference to 'OffloadArch' is ambiguous

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Which compiler is this? Why it works upstream?

@vgvassilev vgvassilev requested a review from lhames August 25, 2025 12:07
Copy link

github-actions bot commented Aug 25, 2025

✅ With the latest revision this PR passed the C/C++ code formatter.

@@ -33,6 +33,10 @@
using namespace llvm;
using namespace llvm::orc;

#if LLVM_ON_UNIX
static std::vector<pid_t> LaunchedExecutorPID;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That should be a member of the Interpreter class I think.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This vector is updated in launchExecutor. And launchExecutor is called before creation of interpreter. I can define it as static but I don't think it is related enough to be in interpreter class.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We need a home for this. The process is not the right home. Either the Interpreter class or the IncrementalExecutor should be good options.

@kr-2003 kr-2003 requested a review from vgvassilev August 26, 2025 15:12
Comment on lines 131 to 161
if ((SystemTriple.isOSBinFormatELF() || SystemTriple.isOSBinFormatMachO())) {
std::string OOPExecutor = getExecutorPath();
std::string OrcRuntimePath = getOrcRuntimePath(CompilerRTPath);
bool UseSharedMemory = false;
std::string SlabAllocateSizeString = "";
std::unique_ptr<llvm::orc::ExecutorProcessControl> EPC;
EPC = ExitOnError(launchExecutor(OOPExecutor, UseSharedMemory,
SlabAllocateSizeString,
[=] { // Lambda defined inline
auto redirect = [](int from, int to) {
if (from != to) {
dup2(from, to);
close(from);
}
};

redirect(0, STDIN_FILENO);
redirect(1, STDOUT_FILENO);
redirect(2, STDERR_FILENO);

setvbuf(stdout, nullptr, _IONBF, 0);
setvbuf(stderr, nullptr, _IONBF, 0);
}));
if (EPC) {
CB.SetTargetTriple(EPC->getTargetTriple().getTriple());
JB = ExitOnError(clang::Interpreter::createLLJITBuilder(std::move(EPC),
OrcRuntimePath));
}
}

return cantFail(clang::Interpreter::create(std::move(CI), std::move(JB)));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All of that should go in Interpreter.cpp logic when IsOutOfProcess is set. This way we won't have to drag OrcRuntimePath around.

@kr-2003 kr-2003 requested a review from vgvassilev August 27, 2025 16:12
namespace clang {

/// \brief Configuration options for out-of-process JIT execution.
struct OutOfProcessJITConfig {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That struct can go as a nested Interpreter class.

bool UseSharedMemory;

/// String representing the slab allocation size for memory management.
std::string SlabAllocateSizeString;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should be an unsigned int I think. All of these should have good default values.

@@ -55,6 +57,8 @@
#include "llvm/TargetParser/Host.h"
#include "llvm/Transforms/Utils/Cloning.h" // for CloneModule

#include <iostream>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We probably do not need this include.

std::unique_ptr<llvm::orc::ExecutorProcessControl> EPC;
if (OutOfProcessConfig.OOPExecutor != "") {
// Launch an out-of-process executor locally in a child process.
EPC = ExitOnErr(launchExecutor(OutOfProcessConfig.OOPExecutor,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remember this is the library, it must not abort the process. Instead the interface should return llvm::Expected<std::unique_ptr<llvm::orc::LLJITBuilder>> and propagate the error up.

Comment on lines +500 to +501
llvm::SmallString<256> BasePath(llvm::sys::fs::getMainExecutable(
"clang-repl", reinterpret_cast<void *>(&getOrcRuntimePath)));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That does not work when we are in a library. Why do we need the BasePath at all? I'd think we can ask the TC and that is mostly it. Do you try to get somehow to the source directory?

@@ -33,6 +33,10 @@
using namespace llvm;
using namespace llvm::orc;

#if LLVM_ON_UNIX
static std::vector<pid_t> LaunchedExecutorPID;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We need a home for this. The process is not the right home. Either the Interpreter class or the IncrementalExecutor should be good options.

#include <optional>

#include <iostream>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We don't need that.

Comment on lines +334 to +337
if (::OffloadArch.empty()) {
::OffloadArch = "sm_35";
}
CB.SetOffloadArch(OffloadArch);
CB.SetOffloadArch(::OffloadArch);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Which compiler is this? Why it works upstream?

} else {
Interp = ExitOnErr(
clang::Interpreter::create(std::move(CI), OutOfProcessConfig));
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants