[clang-repl] Expose RuntimeInterfaceBuilder to allow customization #83126
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
llvmbot
added
clang
|
@llvm/pr-subscribers-clang Author: Stefan Gränitz (weliveindetail) ChangesRuntimeInterfaceBuilder wires up JITed expressions with the hardcoded Interpreter runtime. It's used only for value printing right now, but it is not limited to that. The default implementation focuses on an evaluation process where the Interpreter has direct access to the memory of JITed expressions, because they share the same memory space (in-process or shared memory). We need a different approach to support out-of-process evaluation or variations of the runtime. It seems reasonable to expose a minimal interface for it. The new RuntimeInterfaceBuilder is an abstract base class in the public header. For that, the TypeVisitor had to become a component (instead of inheriting from it). FindRuntimeInterface() was adjusted to return an instance of the RuntimeInterfaceBuilder and it can be overridden from derived classes. Full diff: https://github.com/llvm/llvm-project/pull/83126.diff 2 Files Affected:
diff --git a/clang/include/clang/Interpreter/Interpreter.h b/clang/include/clang/Interpreter/Interpreter.h
index 292fa566ae7037..787357ea7f20c3 100644
--- a/clang/include/clang/Interpreter/Interpreter.h
+++ b/clang/include/clang/Interpreter/Interpreter.h
@@ -18,6 +18,7 @@
#include "clang/AST/GlobalDecl.h"
#include "clang/Interpreter/PartialTranslationUnit.h"
#include "clang/Interpreter/Value.h"
+#include "clang/Sema/Ownership.h"
#include "llvm/ADT/DenseMap.h"
#include "llvm/ExecutionEngine/JITSymbol.h"
@@ -72,17 +73,22 @@ class IncrementalCompilerBuilder {
llvm::StringRef CudaSDKPath;
};
+class RuntimeInterfaceBuilder {
+public:
+ virtual ~RuntimeInterfaceBuilder() = default;
+ virtual ExprResult getCall(Expr *E, ArrayRef<Expr *> FixedArgs) = 0;
+};
+
/// Provides top-level interfaces for incremental compilation and execution.
class Interpreter {
std::unique_ptr<llvm::orc::ThreadSafeContext> TSCtx;
std::unique_ptr<IncrementalParser> IncrParser;
std::unique_ptr<IncrementalExecutor> IncrExecutor;
+ std::unique_ptr<RuntimeInterfaceBuilder> RuntimeIB;
// An optional parser for CUDA offloading
std::unique_ptr<IncrementalParser> DeviceParser;
- Interpreter(std::unique_ptr<CompilerInstance> CI, llvm::Error &Err);
-
llvm::Error CreateExecutor();
unsigned InitPTUSize = 0;
@@ -91,8 +97,13 @@ class Interpreter {
// printing happens, it's in an invalid state.
Value LastValue;
+protected:
+ Interpreter(std::unique_ptr<CompilerInstance> CI, llvm::Error &Err);
+
+ void finalizeInitPTUStack();
+
public:
- ~Interpreter();
+ virtual ~Interpreter();
static llvm::Expected<std::unique_ptr<Interpreter>>
create(std::unique_ptr<CompilerInstance> CI);
static llvm::Expected<std::unique_ptr<Interpreter>>
@@ -137,11 +148,12 @@ class Interpreter {
Expr *SynthesizeExpr(Expr *E);
+protected:
+ virtual RuntimeInterfaceBuilder *FindRuntimeInterface();
+
private:
size_t getEffectivePTUSize() const;
- bool FindRuntimeInterface();
-
llvm::DenseMap<CXXRecordDecl *, llvm::orc::ExecutorAddr> Dtors;
llvm::SmallVector<Expr *, 4> ValuePrintingInfo;
diff --git a/clang/lib/Interpreter/Interpreter.cpp b/clang/lib/Interpreter/Interpreter.cpp
index 9f97a3c6b0be9e..4dc15f5c735fc3 100644
--- a/clang/lib/Interpreter/Interpreter.cpp
+++ b/clang/lib/Interpreter/Interpreter.cpp
@@ -283,10 +283,12 @@ Interpreter::create(std::unique_ptr<CompilerInstance> CI) {
return PTU.takeError();
Interp->ValuePrintingInfo.resize(4);
+
// FIXME: This is a ugly hack. Undo command checks its availability by looking
// at the size of the PTU list. However we have parsed something in the
// beginning of the REPL so we have to mark them as 'Irrevocable'.
- Interp->InitPTUSize = Interp->IncrParser->getPTUs().size();
+ Interp->finalizeInitPTUStack();
+
return std::move(Interp);
}
@@ -343,6 +345,11 @@ const ASTContext &Interpreter::getASTContext() const {
return getCompilerInstance()->getASTContext();
}
+void Interpreter::finalizeInitPTUStack() {
+ assert(!InitPTUSize && "We only do this once");
+ InitPTUSize = IncrParser->getPTUs().size();
+}
+
size_t Interpreter::getEffectivePTUSize() const {
std::list<PartialTranslationUnit> &PTUs = IncrParser->getPTUs();
assert(PTUs.size() >= InitPTUSize && "empty PTU list?");
@@ -505,9 +512,16 @@ static constexpr llvm::StringRef MagicRuntimeInterface[] = {
"__clang_Interpreter_SetValueWithAlloc",
"__clang_Interpreter_SetValueCopyArr", "__ci_newtag"};
-bool Interpreter::FindRuntimeInterface() {
+static std::unique_ptr<RuntimeInterfaceBuilder>
+createInProcessRuntimeInterfaceBuilder(Interpreter &Interp, ASTContext &Ctx,
+ Sema &S);
+
+RuntimeInterfaceBuilder *Interpreter::FindRuntimeInterface() {
+ if (RuntimeIB)
+ return RuntimeIB.get();
+
if (llvm::all_of(ValuePrintingInfo, [](Expr *E) { return E != nullptr; }))
- return true;
+ return nullptr;
Sema &S = getCompilerInstance()->getSema();
ASTContext &Ctx = S.getASTContext();
@@ -526,43 +540,127 @@ bool Interpreter::FindRuntimeInterface() {
if (!LookupInterface(ValuePrintingInfo[NoAlloc],
MagicRuntimeInterface[NoAlloc]))
- return false;
+ return nullptr;
if (!LookupInterface(ValuePrintingInfo[WithAlloc],
MagicRuntimeInterface[WithAlloc]))
- return false;
+ return nullptr;
if (!LookupInterface(ValuePrintingInfo[CopyArray],
MagicRuntimeInterface[CopyArray]))
- return false;
+ return nullptr;
if (!LookupInterface(ValuePrintingInfo[NewTag],
MagicRuntimeInterface[NewTag]))
- return false;
- return true;
+ return nullptr;
+
+ RuntimeIB = createInProcessRuntimeInterfaceBuilder(*this, Ctx, S);
+ return RuntimeIB.get();
}
namespace {
-class RuntimeInterfaceBuilder
- : public TypeVisitor<RuntimeInterfaceBuilder, Interpreter::InterfaceKind> {
- clang::Interpreter &Interp;
+class InterfaceKindVisitor
+ : public TypeVisitor<InterfaceKindVisitor, Interpreter::InterfaceKind> {
+ friend class InProcessRuntimeInterfaceBuilder;
+
ASTContext &Ctx;
Sema &S;
Expr *E;
llvm::SmallVector<Expr *, 3> Args;
public:
- RuntimeInterfaceBuilder(clang::Interpreter &In, ASTContext &C, Sema &SemaRef,
- Expr *VE, ArrayRef<Expr *> FixedArgs)
- : Interp(In), Ctx(C), S(SemaRef), E(VE) {
- // The Interpreter* parameter and the out parameter `OutVal`.
- for (Expr *E : FixedArgs)
+ InterfaceKindVisitor(ASTContext &Ctx, Sema &S, Expr *E)
+ : Ctx(Ctx), S(S), E(E) {}
+
+ Interpreter::InterfaceKind VisitRecordType(const RecordType *Ty) {
+ return Interpreter::InterfaceKind::WithAlloc;
+ }
+
+ Interpreter::InterfaceKind
+ VisitMemberPointerType(const MemberPointerType *Ty) {
+ return Interpreter::InterfaceKind::WithAlloc;
+ }
+
+ Interpreter::InterfaceKind
+ VisitConstantArrayType(const ConstantArrayType *Ty) {
+ return Interpreter::InterfaceKind::CopyArray;
+ }
+
+ Interpreter::InterfaceKind
+ VisitFunctionProtoType(const FunctionProtoType *Ty) {
+ HandlePtrType(Ty);
+ return Interpreter::InterfaceKind::NoAlloc;
+ }
+
+ Interpreter::InterfaceKind VisitPointerType(const PointerType *Ty) {
+ HandlePtrType(Ty);
+ return Interpreter::InterfaceKind::NoAlloc;
+ }
+
+ Interpreter::InterfaceKind VisitReferenceType(const ReferenceType *Ty) {
+ ExprResult AddrOfE = S.CreateBuiltinUnaryOp(SourceLocation(), UO_AddrOf, E);
+ assert(!AddrOfE.isInvalid() && "Can not create unary expression");
+ Args.push_back(AddrOfE.get());
+ return Interpreter::InterfaceKind::NoAlloc;
+ }
+
+ Interpreter::InterfaceKind VisitBuiltinType(const BuiltinType *Ty) {
+ if (Ty->isNullPtrType())
Args.push_back(E);
+ else if (Ty->isFloatingType())
+ Args.push_back(E);
+ else if (Ty->isIntegralOrEnumerationType())
+ HandleIntegralOrEnumType(Ty);
+ else if (Ty->isVoidType()) {
+ // Do we need to still run `E`?
+ }
+
+ return Interpreter::InterfaceKind::NoAlloc;
+ }
+
+ Interpreter::InterfaceKind VisitEnumType(const EnumType *Ty) {
+ HandleIntegralOrEnumType(Ty);
+ return Interpreter::InterfaceKind::NoAlloc;
+ }
+private:
+ // Force cast these types to uint64 to reduce the number of overloads of
+ // `__clang_Interpreter_SetValueNoAlloc`.
+ void HandleIntegralOrEnumType(const Type *Ty) {
+ TypeSourceInfo *TSI = Ctx.getTrivialTypeSourceInfo(Ctx.UnsignedLongLongTy);
+ ExprResult CastedExpr =
+ S.BuildCStyleCastExpr(SourceLocation(), TSI, SourceLocation(), E);
+ assert(!CastedExpr.isInvalid() && "Cannot create cstyle cast expr");
+ Args.push_back(CastedExpr.get());
+ }
+
+ void HandlePtrType(const Type *Ty) {
+ TypeSourceInfo *TSI = Ctx.getTrivialTypeSourceInfo(Ctx.VoidPtrTy);
+ ExprResult CastedExpr =
+ S.BuildCStyleCastExpr(SourceLocation(), TSI, SourceLocation(), E);
+ assert(!CastedExpr.isInvalid() && "Can not create cstyle cast expression");
+ Args.push_back(CastedExpr.get());
+ }
+};
+
+class InProcessRuntimeInterfaceBuilder : public RuntimeInterfaceBuilder {
+ Interpreter &Interp;
+ ASTContext &Ctx;
+ Sema &S;
+
+public:
+ InProcessRuntimeInterfaceBuilder(Interpreter &Interp, ASTContext &C, Sema &S)
+ : Interp(Interp), Ctx(C), S(S) {}
+
+ ExprResult getCall(Expr *E, ArrayRef<Expr *> FixedArgs) override {
// Get rid of ExprWithCleanups.
if (auto *EWC = llvm::dyn_cast_if_present<ExprWithCleanups>(E))
E = EWC->getSubExpr();
- }
- ExprResult getCall() {
+ InterfaceKindVisitor Visitor(Ctx, S, E);
+
+ // The Interpreter* parameter and the out parameter `OutVal`.
+ for (Expr *E : FixedArgs)
+ Visitor.Args.push_back(E);
+
QualType Ty = E->getType();
QualType DesugaredTy = Ty.getDesugaredType(Ctx);
@@ -575,11 +673,11 @@ class RuntimeInterfaceBuilder
Expr *TypeArg =
CStyleCastPtrExpr(S, Ctx.VoidPtrTy, (uintptr_t)Ty.getAsOpaquePtr());
// The QualType parameter `OpaqueType`, represented as `void*`.
- Args.push_back(TypeArg);
+ Visitor.Args.push_back(TypeArg);
// We push the last parameter based on the type of the Expr. Note we need
// special care for rvalue struct.
- Interpreter::InterfaceKind Kind = Visit(&*DesugaredTy);
+ Interpreter::InterfaceKind Kind = Visitor.Visit(&*DesugaredTy);
switch (Kind) {
case Interpreter::InterfaceKind::WithAlloc:
case Interpreter::InterfaceKind::CopyArray: {
@@ -587,7 +685,7 @@ class RuntimeInterfaceBuilder
ExprResult AllocCall = S.ActOnCallExpr(
/*Scope=*/nullptr,
Interp.getValuePrintingInfo()[Interpreter::InterfaceKind::WithAlloc],
- E->getBeginLoc(), Args, E->getEndLoc());
+ E->getBeginLoc(), Visitor.Args, E->getEndLoc());
assert(!AllocCall.isInvalid() && "Can't create runtime interface call!");
TypeSourceInfo *TSI = Ctx.getTrivialTypeSourceInfo(Ty, SourceLocation());
@@ -634,85 +732,21 @@ class RuntimeInterfaceBuilder
return S.ActOnCallExpr(
/*Scope=*/nullptr,
Interp.getValuePrintingInfo()[Interpreter::InterfaceKind::NoAlloc],
- E->getBeginLoc(), Args, E->getEndLoc());
+ E->getBeginLoc(), Visitor.Args, E->getEndLoc());
}
default:
llvm_unreachable("Unhandled Interpreter::InterfaceKind");
}
}
-
- Interpreter::InterfaceKind VisitRecordType(const RecordType *Ty) {
- return Interpreter::InterfaceKind::WithAlloc;
- }
-
- Interpreter::InterfaceKind
- VisitMemberPointerType(const MemberPointerType *Ty) {
- return Interpreter::InterfaceKind::WithAlloc;
- }
-
- Interpreter::InterfaceKind
- VisitConstantArrayType(const ConstantArrayType *Ty) {
- return Interpreter::InterfaceKind::CopyArray;
- }
-
- Interpreter::InterfaceKind
- VisitFunctionProtoType(const FunctionProtoType *Ty) {
- HandlePtrType(Ty);
- return Interpreter::InterfaceKind::NoAlloc;
- }
-
- Interpreter::InterfaceKind VisitPointerType(const PointerType *Ty) {
- HandlePtrType(Ty);
- return Interpreter::InterfaceKind::NoAlloc;
- }
-
- Interpreter::InterfaceKind VisitReferenceType(const ReferenceType *Ty) {
- ExprResult AddrOfE = S.CreateBuiltinUnaryOp(SourceLocation(), UO_AddrOf, E);
- assert(!AddrOfE.isInvalid() && "Can not create unary expression");
- Args.push_back(AddrOfE.get());
- return Interpreter::InterfaceKind::NoAlloc;
- }
-
- Interpreter::InterfaceKind VisitBuiltinType(const BuiltinType *Ty) {
- if (Ty->isNullPtrType())
- Args.push_back(E);
- else if (Ty->isFloatingType())
- Args.push_back(E);
- else if (Ty->isIntegralOrEnumerationType())
- HandleIntegralOrEnumType(Ty);
- else if (Ty->isVoidType()) {
- // Do we need to still run `E`?
- }
-
- return Interpreter::InterfaceKind::NoAlloc;
- }
-
- Interpreter::InterfaceKind VisitEnumType(const EnumType *Ty) {
- HandleIntegralOrEnumType(Ty);
- return Interpreter::InterfaceKind::NoAlloc;
- }
-
-private:
- // Force cast these types to uint64 to reduce the number of overloads of
- // `__clang_Interpreter_SetValueNoAlloc`.
- void HandleIntegralOrEnumType(const Type *Ty) {
- TypeSourceInfo *TSI = Ctx.getTrivialTypeSourceInfo(Ctx.UnsignedLongLongTy);
- ExprResult CastedExpr =
- S.BuildCStyleCastExpr(SourceLocation(), TSI, SourceLocation(), E);
- assert(!CastedExpr.isInvalid() && "Cannot create cstyle cast expr");
- Args.push_back(CastedExpr.get());
- }
-
- void HandlePtrType(const Type *Ty) {
- TypeSourceInfo *TSI = Ctx.getTrivialTypeSourceInfo(Ctx.VoidPtrTy);
- ExprResult CastedExpr =
- S.BuildCStyleCastExpr(SourceLocation(), TSI, SourceLocation(), E);
- assert(!CastedExpr.isInvalid() && "Can not create cstyle cast expression");
- Args.push_back(CastedExpr.get());
- }
};
} // namespace
+static std::unique_ptr<RuntimeInterfaceBuilder>
+createInProcessRuntimeInterfaceBuilder(Interpreter &Interp, ASTContext &Ctx,
+ Sema &S) {
+ return std::make_unique<InProcessRuntimeInterfaceBuilder>(Interp, Ctx, S);
+}
+
// This synthesizes a call expression to a speciall
// function that is responsible for generating the Value.
// In general, we transform:
@@ -731,8 +765,8 @@ Expr *Interpreter::SynthesizeExpr(Expr *E) {
Sema &S = getCompilerInstance()->getSema();
ASTContext &Ctx = S.getASTContext();
- if (!FindRuntimeInterface())
- llvm_unreachable("We can't find the runtime iterface for pretty print!");
+ RuntimeInterfaceBuilder *Builder = FindRuntimeInterface();
+ assert(Builder && "We can't find the runtime interface for pretty print!");
// Create parameter `ThisInterp`.
auto *ThisInterp = CStyleCastPtrExpr(S, Ctx.VoidPtrTy, (uintptr_t)this);
@@ -741,9 +775,8 @@ Expr *Interpreter::SynthesizeExpr(Expr *E) {
auto *OutValue = CStyleCastPtrExpr(S, Ctx.VoidPtrTy, (uintptr_t)&LastValue);
// Build `__clang_Interpreter_SetValue*` call.
- RuntimeInterfaceBuilder Builder(*this, Ctx, S, E, {ThisInterp, OutValue});
+ ExprResult Result = Builder->getCall(E, {ThisInterp, OutValue});
- ExprResult Result = Builder.getCall();
// It could fail, like printing an array type in C. (not supported)
if (Result.isInvalid())
return E;
|
weliveindetail
commented
Feb 27, 2024
There was a problem hiding this comment.
This is an early proposal. I'd like to collect feedback before polishing. Let's focus on the conceptual questions. I tried to keep the patch minimal, so that it's easier to review. Each of the three commits depends on the previous ones. They should work and be reviewed in isolation (I guess). What do you think?
| @@ -741,9 +775,8 @@ Expr *Interpreter::SynthesizeExpr(Expr *E) { | |||
| auto *OutValue = CStyleCastPtrExpr(S, Ctx.VoidPtrTy, (uintptr_t)&LastValue); | |||
|
|
|||
| // Build `__clang_Interpreter_SetValue*` call. | |||
| RuntimeInterfaceBuilder Builder(*this, Ctx, S, E, {ThisInterp, OutValue}); | |||
| ExprResult Result = Builder->getCall(E, {ThisInterp, OutValue}); | |||
There was a problem hiding this comment.
FindRuntimeInterface() and getCall() are virtual functions now, which introduces a tiny runtime overhead for each synthesized expression. It should be negligible compared to the effort for parsing and compilation, but if it's an issue, I am happy to change it to a call to function pointer.
|
I was wondering if we can extend that functionality via a callback mechanism which should be cheaper. We also have some pending work (which I was planning on working when time permits) in that area D146809. I was wondering if that'd interfere with your work... |
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
|
Alright. The above change makes it as cheap as it gets with an abstraction: virtual function calls only on init and one function pointer call for each processed statement.
How would that look like? I assume the runtime interface will have other functions than We can change it to provide function pointers instead like in my last patch. It's less idiomatic but a bit cheaper. Of course we could use
Your patch doesn't actually touch the |
weliveindetail
force-pushed
the
clang-repl-runtimeib-main
branch
from
b4b71a1 to
8ba5253
Compare
|
Squashed, rebased, polished and tested |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
RuntimeInterfaceBuilder wires up JITed expressions with the hardcoded Interpreter runtime. It's used only for value printing right now, but it is not limited to that. The default implementation focuses on an evaluation process where the Interpreter has direct access to the memory of JITed expressions, because they share the same memory space (in-process or shared memory).
We need a different approach to support out-of-process evaluation or variations of the runtime. It seems reasonable to expose a minimal interface for it. The new RuntimeInterfaceBuilder is an abstract base class in the public header. For that, the TypeVisitor had to become a component (instead of inheriting from it). FindRuntimeInterface() was adjusted to return an instance of the RuntimeInterfaceBuilder and it can be overridden from derived classes.