[PATCH] D64146: [Clang Interpreter] Initial patch for the constexpr interpreter
Richard Smith - zygoloid via Phabricator via cfe-commits
cfe-commits at lists.llvm.org
Mon Aug 12 16:02:38 PDT 2019
rsmith added a comment.
I would like to better understand the big picture for descriptors, pointers, and so on. I'm not yet seeing how the pieces are going to fit together and not frequently require expensive operations. For example, pointer arithmetic requires determining the array bound of the pointee; pointer subtraction requires determining whether two pointers point into the same array; pointer comparisons require determining the divergence point in the access paths between two pointers. These path-based operations were easy in the old representation, and seem to be difficult in the new representation, so I want to make sure that they've been properly considered. It's also not clear to me how you'll model pointers that have lost their designator (through constant folding), where you have a runtime offset but no compile-time offset, or pointers that point to objects whose values are not part of the evaluation (for example, `extern` globals), or pointers that have a designator but no base (for `__builtin_object_size` handling).
This choice of representation also seems to make it really easy for a minor bug in the interpreter to turn into undefined behavior during compilation. Have you given any thought to how that might be avoided or mitigated?
================
Comment at: clang/include/clang/Basic/LangOptions.def:291-294
+BENIGN_LANGOPT(EnableClangInterp, 1, 0,
+ "enable the experimental clang interpreter")
+BENIGN_LANGOPT(ForceClangInterp, 1, 0,
+ "force the use of the experimental constexpr interpreter")
----------------
"Clang" is redundant here (what else could it be?). If we're calling the new thing "interp" internally, then I guess "EnableInterp" and "ForceInterp" seem OK, but given that this is just scaffolding until the new interpreter is done, maybe "EnableNewInterp" and "ForceNewInterp" would be better. Suggestion:
```
BENIGN_LANGOPT(EnableNewInterp, 1, 0,
"enable the experimental new constexpr interpreter")
BENIGN_LANGOPT(ForceNewInterp, 1, 0,
"force the use of the experimental new constexpr interpreter")
```
================
Comment at: clang/include/clang/Driver/Options.td:836-839
+def fexperimental_clang_interpreter : Flag<["-"], "fexperimental-clang-interpreter">, Group<f_Group>,
+ HelpText<"Enable the experimental clang interpreter">, Flags<[CC1Option]>;
+def fforce_experimental_clang_interpreter : Flag<["-"], "fforce-experimental-clang-interpreter">, Group<f_Group>,
+ HelpText<"Force the use of the experimental clang interpreter, failing on missing features">, Flags<[CC1Option]>;
----------------
On reflection, let's put "new-" in here too, to match `-fexperimental-new-pass-manager` (so `-fexperimental-new-constexpr-interpreter`). (As above, this flag name should not contain the string "clang".)
================
Comment at: clang/lib/AST/CMakeLists.txt:85-87
+ clangInterp
clangBasic
clangLex
----------------
Please keep these in alphabetical order.
================
Comment at: clang/lib/AST/ExprConstant.cpp:55-56
#include "llvm/ADT/SmallBitVector.h"
+#include "clang/Basic/OptionalDiagnostic.h"
+#include "clang/Basic/TargetInfo.h"
#include "llvm/Support/SaveAndRestore.h"
----------------
Please keep these in alphabetical order.
================
Comment at: clang/lib/AST/ExprConstant.cpp:5039-5040
+ return true;
+ case interp::InterpResult::Fail:
+ return false;
+ case interp::InterpResult::Bail:
----------------
Missing a check for `ForceClangInterp` here.
================
Comment at: clang/lib/AST/ExprConstant.cpp:12135-12136
+ return true;
+ case interp::InterpResult::Fail:
+ return false;
+ case interp::InterpResult::Bail:
----------------
Missing a check for `ForceClangInterp` here.
================
Comment at: clang/lib/AST/ExprConstant.cpp:12362-12364
+ // If the interpreter is forced, do not compute any other value.
+ if (Info.ForceClangInterp)
+ return true;
----------------
Isn't this in the wrong place? If interp succeeds, I think we always want to stop; it's only if interp fails / bails that `ForceClangInterp` should matter. Or did I misunderstand what that flag is for?
================
Comment at: clang/lib/AST/ExprVM/Pointer.h:30
+
+/// Describes a memory block - generated once by the compiler.
+struct Descriptor {
----------------
nand wrote:
> rsmith wrote:
> > How is this going to work for subobjects? We can't generate the full space of all possible descriptors, as that would be unmanageable for large arrays of complicated types.
> Subobjects will have pointers to descriptors embedded into their data - a pointer inside a block can follow that chain of descriptors up to the root.
I don't think I understand what you're suggesting here. If I have, for example:
```
struct X { char c; };
X x[1000000];
```
... are you saying you would embed 1000000 descriptors into the representation of `x`? That seems extremely wasteful to me. Presumably the goal should be to get as close to allocating 1000000 bytes of storage for `x` as possible. I think we can probably get down to 1000000 bytes for the `c` members plus perhaps 1 bit for each `X` object (to track whether it's within its lifetime) and 2 bits for each `char` object (to track whether it's within its lifetime and whether it's initialized), for a 1.375x storage overhead in this case. But I don't see how you get there from this representation.
================
Comment at: clang/lib/AST/Interp/ByteCodeGen.cpp:54
+
+ virtual void emitDestructor() {}
+
----------------
I think `emitDestruction` would probably be a better name, since this needs to encompass things that are not destructors (such as invalidating pointers/references that point into the destroyed objects).
================
Comment at: clang/lib/AST/Interp/ByteCodeGen.cpp:99
+
+template <class Emitter> class BlockScope final : public LocalScope<Emitter> {
+public:
----------------
Doc comment please. "Block" means two different things in Clang (compound statements and the Apple Blocks extension) and a reader needs to know which one.
================
Comment at: clang/lib/AST/Interp/ByteCodeGen.cpp:119
+
+/// Scope used for toplevel declarators.
+template <class Emitter> class DeclScope final : public LocalScope<Emitter> {
----------------
I don't know what this means. Declarators don't typically introduce a scope (for lifetime purposes).
Do you mean "for the initializer of a top-level variable declaration" or something like that?
================
Comment at: clang/lib/AST/Interp/ByteCodeGen.cpp:145-153
+ if (auto *Exp = dyn_cast<Expr>(S)) {
+ if (Exp->getType()->isVoidType()) {
+ return this->Visit(Exp);
+ } else {
+ return discard(Exp);
+ }
+ } else {
----------------
No `else` after `return`, please:
```
if (auto *Exp = dyn_cast<Expr>(S)) {
if (Exp->getType()->isVoidType())
return this->Visit(Exp);
return discard(Exp);
}
return this->bail(S);
```
================
Comment at: clang/lib/AST/Interp/ByteCodeGen.cpp:161
+ BlockScope<Emitter> Scope(this);
+ for (auto *Stmt : S->body())
+ if (!visit(Stmt))
----------------
No `auto` here, and please don't shadow `clang::Stmt` with a local variable.
================
Comment at: clang/lib/AST/Interp/ByteCodeGen.cpp:186
+bool ByteCodeGen<Emitter>::visitReturnStmt(const ReturnStmt *RS) {
+ if (auto *RetExpr = RS->getRetValue()) {
+ ExprScope<Emitter> RetScope(this);
----------------
No `auto` here, please.
================
Comment at: clang/lib/AST/Interp/ByteCodeGen.cpp:206
+ } else {
+ for (auto *C = Top; C; C = C->getParent())
+ C->emitDestructor();
----------------
No `auto` here, please.
================
Comment at: clang/lib/AST/Interp/ByteCodeGen.cpp:217
+ BlockScope<Emitter> IfScope(this);
+ if (auto *CondInit = IS->getInit())
+ if (!visit(IS->getInit()))
----------------
No `auto` here, please.
================
Comment at: clang/lib/AST/Interp/ByteCodeGen.cpp:221
+
+ if (auto *CondDecl = IS->getConditionVariableDeclStmt())
+ if (!visitDeclStmt(CondDecl))
----------------
No `auto` here, please.
================
Comment at: clang/lib/AST/Interp/ByteCodeGen.cpp:229-230
+ if (auto *Else = IS->getElse()) {
+ auto LabelElse = this->getLabel();
+ auto LabelEnd = this->getLabel();
+ if (!this->jumpFalse(LabelElse))
----------------
Especially no `auto` here. Even someone familiar with the rest of the clang codebase won't be able to guess these types. In particular, as a reader, I'm wondering if this is some type provided by `Emitter` or whether it's some type provided by `ByteCodeGen`, and what the contract is between this class and its template argument with regard to this type. Spelling this as `typename Emitter::Label` would help substantially.
I'm not going to point out any more uses, but generally if you only use `auto` when the type is obvious from the spelling of the initializer (eg, when the initializer is `dyn_cast<T>(...)`), that would be fine.
================
Comment at: clang/lib/AST/Interp/ByteCodeGen.cpp:256
+ if (auto *PD = dyn_cast<ParmVarDecl>(DE->getDecl())) {
+ auto It = this->Params.find(PD);
+ if (It == this->Params.end()) {
----------------
(This uses of `auto` like this are also fine; it's obvious this is an iterator and we don't need to know more than that.)
================
Comment at: clang/lib/AST/Interp/ByteCodeGen.cpp:352-381
+ case BO_LAnd: {
+ if (!this->Visit(LHS))
+ return false;
+ auto Short = this->getLabel();
+ if (!this->emitDup(*TypeLHS, BO))
+ return false;
+ if (!this->jumpFalse(Short))
----------------
Have you considered how you will support Clang's current constant folding of `&&` and `||` expressions where the left-hand side is non-constant but the right hand side is a false or true constant (respectively)?
================
Comment at: clang/lib/AST/Interp/ByteCodeGen.cpp:536
+ } else {
+ // If the param is a pointer, we can dereference a dummy value.
+ if (PD->getType()->hasPointerRepresentation()) {
----------------
We can, but should we?
I would think the only time we can reach this case is when we're constant-evaluating an expression within a function and that expression names a function parameter, and in that case, the expression should be treated as non-constant. We should have a more direct way to represent "if you get here, the evaluation is non-constant, with this reason" in the bytecode.
================
Comment at: clang/lib/AST/Interp/ByteCodeGen.cpp:546
+ }
+ if (auto *VD = dyn_cast<VarDecl>(DE->getDecl())) {
+ auto It = Locals.find(VD);
----------------
The above `ParmVarDecl` case can fall through into here, because a `ParmVarDecl` is a `VarDecl`. Is that what you intended?
Do you actually need to treat locals and parameters separately? This would seem simpler if you could model them as being the same thing.
================
Comment at: clang/lib/AST/Interp/ByteCodeGen.cpp:600-601
+ auto VT = VD->getType();
+ if (VT.isConstQualified() && VT->isFundamentalType()) {
+ if (AK == AccessKind::Read) {
+ if (!this->Visit(VD->getInit()))
----------------
Using an `&&` here rather than a nested `if` would help readability a little.
================
Comment at: clang/lib/AST/Interp/ByteCodeGen.cpp:605-611
+ }
+ }
+ }
+ }
+ }
+ }
+ }
----------------
Please try to find some way to refactor this so that there's not so many nested scopes; it makes it hard to follow the control flow within the function.
================
Comment at: clang/lib/AST/Interp/ByteCodeGen.cpp:647-662
+ case PT_Sint8:
+ return this->emitConstSint8(Value.getSExtValue(), E);
+ case PT_Uint8:
+ return this->emitConstUint8(Value.getZExtValue(), E);
+ case PT_Sint16:
+ return this->emitConstSint16(Value.getSExtValue(), E);
+ case PT_Uint16:
----------------
I'd like to see this generated from a `.def` file, rather than enumerating the various currently-supported integer types in many different places. The goal should be that adding support for a new integer type / size (say we start supporting a target with 48 bit integers) should require making changes in only one place. Case in point: you're currently missing support for 128-bit integers, which Clang does support across many targets.
================
Comment at: clang/lib/AST/Interp/ByteCodeGen.cpp:718-720
+ return this->emitRetVoid({});
+ else
+ return this->emitNoRet({});
----------------
These `{}` arguments are completely opaque here. Please add some kind of a clue as to what you're passing in here. (Can you use `SourceInfo()`?)
================
Comment at: clang/lib/AST/Interp/ByteCodeGen.h:40
+class ByteCodeGen : public ConstStmtVisitor<ByteCodeGen<Emitter>, bool>,
+ public Emitter {
+ using NullaryFn = bool (ByteCodeGen::*)(const SourceInfo &);
----------------
Does `Emitter` need to be a base class rather than a member?
================
Comment at: clang/lib/AST/Interp/ByteCodeGen.h:111-113
+ virtual bool compile(const FunctionDecl *F) override;
+ virtual bool compile(const Expr *E) override;
+ virtual bool compile(const VarDecl *VD) override;
----------------
Is there any reason these need to be `virtual`? It looks like you would never call these from a context where the concrete type is unknown. (These are the only implementations of the pure virtual functions they override.)
================
Comment at: clang/lib/AST/Interp/Descriptor.h:29-31
+using BlockCtorFn = void (*)(Block *B, char *, bool, Descriptor *);
+using BlockDtorFn = void (*)(Block *B, char *, Descriptor *);
+using BlockMoveFn = void (*)(Block *B, char *, char *, Descriptor *);
----------------
Parameter names here would help explain what these functions are supposed to do -- particularly for `BlockMoveFn` (which `char*` is which?), but also for the `bool` parameter of `BlockCtorFn`.
================
Comment at: clang/lib/AST/Interp/Descriptor.h:62
+ const BlockDtorFn DtorFn = nullptr;
+ /// Invoked when a block with pointers referencing it goes out of scoped. Such
+ /// blocks are persisted: the move function copies all inline descriptors and
----------------
scoped -> scope
================
Comment at: clang/lib/AST/Interp/EvalEmitter.cpp:71-75
+bool EvalEmitter::bail(const SourceLocation &Loc) {
+ if (!BailLocation)
+ BailLocation = Loc;
+ return false;
+}
----------------
It seems like a problem that an unsupported construct in a `!isActive()` context causes us to fail evaluation.
================
Comment at: clang/lib/AST/Interp/EvalEmitter.cpp:105-111
+template <PrimType OpType> bool EvalEmitter::emitRet(const SourceInfo &Info) {
+ if (!isActive()) {
+ return true;
+ }
+ using T = typename PrimConv<OpType>::T;
+ return ReturnValue<T>(S.Stk.pop<T>(), Result);
+}
----------------
Can this happen? I would expect not: if we can't cope with backwards jumps here, I wouldn't expect us to be able to cope with function calls / returns.
If this can happen, we need to skip anything that happens after the `return` rather than keeping on evaluating it.
================
Comment at: clang/lib/AST/Interp/EvalEmitter.h:69
+ bool jump(const LabelTy &Label);
+ bool fallthrough(const LabelTy &Label);
+
----------------
I can't find any calls to this, and I'm not sure what it's for. Is it necessary?
================
Comment at: clang/lib/AST/Interp/EvalEmitter.h:112-114
+ /// Since expressions can only jump forward, predicated execution is
+ /// used to deal with if-else statements.
+ bool isActive() { return CurrentLabel == ActiveLabel; }
----------------
I'm curious how much this approach costs us, compared to allowing the emitter to tell `ByteCodeGen` whether to emit both arms of a `?:` (or if not, telling it which one to follow) and similarly for `&&` / `||`. There's the constant overhead of checking `isActive()` on each action, plus the cost of the calls into `EvalEmitter` while "evaluating" the unselected operand. (Eg, `false ? some-very-large-expression : 0` would be evaluated much faster if we could tell `ByteCodeGen` not to bother with the unselected operand.)
(This approach doesn't support statement-expressions, because there can be backwards jumps in those, so I think `?:`/`&&`/`||` are pretty much the only things this supports, right?)
================
Comment at: clang/lib/AST/Interp/Interp.h:213-222
+ if (!Pointer::isComparable(LHS, RHS)) {
+ const auto &Loc = S.Current->getSource(OpPC);
+ S.FFDiag(Loc, diag::note_invalid_subexpr_in_const_expr);
+ return false;
+ } else {
+ auto VL = LHS.getOffset();
+ auto VR = RHS.getOffset();
----------------
This is missing lots of necessary checks; I think your chosen model for pointers makes these checks quite hard to implement. The validity of pointer comparisons in constant evaluation depends on the point of divergence of the two access paths. For example:
```
struct A { int n; };
struct B : A { int m; } b;
B ba[10];
constexpr bool x = ba[0].n < &ba[0].m; // error, not constant, divergence point is base class
constexpr bool y = ba[0].n < &ba[1].m; // ok, divergence point is array indexing
```
Also, access control must be taken into account:
```
struct A {
int x;
private:
int y;
constexpr bool q(A a) { return &a.x < &a.y; } // not constant, x and y have different access
};
```
================
Comment at: clang/lib/AST/Interp/Interp.h:218-219
+ } else {
+ auto VL = LHS.getOffset();
+ auto VR = RHS.getOffset();
+ S.Stk.push<BoolT>(BoolT::from(Fn(Compare(VL, VR))));
----------------
On what basis do you assume that the orders of the offsets in the compile-time representation has any relation to the correct order for a pointer comparison? Do you intend to guarantee this when doing struct layout for the interpreter? If so, please include a comment here explaining that.
================
Comment at: clang/lib/AST/Interp/InterpFrame.cpp:25-36
+ if (Func) {
+ if (auto FrameSize = Func->getFrameSize()) {
+ Locals = llvm::make_unique<char[]>(FrameSize);
+ for (auto &Scope : Func->scopes()) {
+ for (auto &Local : Scope.locals()) {
+ Block *B = new (localBlock(Local.Offset)) Block(Local.Desc);
+ if (Local.Desc->CtorFn)
----------------
It seems wasteful to me to do this initialization work when entering the frame rather than when reaching the declaration of the variable in question. Do we need to do it early like this?
How does this deal with loops? I would have expected that we'd re-initialize a loop scope each time we enter the loop body, so that we don't confuse variables from one iteration with variables from another.
================
Comment at: clang/lib/AST/Interp/LinkEmitter.h:32
+/// An emitter which links the program to bytecode for later use.
+class LinkEmitter {
+protected:
----------------
Should this be called `ByteCodeEmitter` if it emits byte code?
================
Comment at: clang/lib/AST/Interp/Opcodes.td:1
+//===--- Opcodes.td - Opcode defitions for the constexpr VM -----*- C++ -*-===//
+//
----------------
This still needs to be expanded to actually describe what the opcodes do (that is, their effects on the stack and any side-effects they cause). Something simple like:
```
// [RetVal] -> [], returns RetVal
def Ret : Opcode {
```
```
// [A, B] -> [B - A]
def Sub : AluRealOpcode;
```
would be enough.
================
Comment at: clang/lib/AST/Interp/Pointer.h:69-78
+ /// Start of the chain of pointers.
+ Pointer *Pointers = nullptr;
+ /// Flag indicating if the block has static storage duration.
+ bool IsStatic = false;
+ /// Flag indicating if the block is an extern.
+ bool IsExtern = false;
+ /// Flag indicating if the pointer is dead.
----------------
If we're allocating one of these for every storage allocation in the evaluation, packing these bools into the spare bits in the pointers seems worthwhile.
================
Comment at: clang/lib/AST/Interp/Pointer.h:245-249
+ /// Returns the embedded descriptor preceding a field.
+ InlineDescriptor *getInlineDescriptor() const {
+ assert(Base != 0 && "Not a nested pointer");
+ return reinterpret_cast<InlineDescriptor *>(Pointee->data() + Base) - 1;
+ }
----------------
What is this for? I think we need a design document here describing the intended model for pointers, references, descriptors, and so on -- preferably something that can be included in the internals manual.
================
Comment at: clang/lib/AST/Interp/Pointer.h:251-261
+ /// The block the pointer is pointing to.
+ Block *Pointee = nullptr;
+ /// Start of the current subfield.
+ unsigned Base = 0;
+ /// Offset into the block.
+ unsigned Offset = 0;
+
----------------
You will also need to track at least:
* Whether this is a one-past-the-end pointer
* (Information equivalent to) which union members were accessed on the path to this point
I think everything else can be reconstructed from the type and offset.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D64146/new/
https://reviews.llvm.org/D64146
More information about the cfe-commits
mailing list