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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
