[PATCH] D64146: [ConstExprPreter][WIP] Initial patch for the constexpr interpreter

[Nandor Licker via Phabricator via cfe-commits]

nand marked 40 inline comments as done.
nand added a comment.

I have applied most of the changes you suggested to my HEAD which had 
significantly more functionality, including a replacement of Opcodes.in with 
TableGen.
Quite a few of your concerns are answered by the features I have added between 
submitting the patch and now. The interpreter now stands at ~6k LOC.

Should I update the diff with an interpreter trimmed down to the functionality 
of the current diff (~3k LOC) or post the full interpreter now? What would be 
easier to review?




Comment at: clang/lib/AST/ExprVM/Compiler.cpp:88
+  Params.insert({ParamDecl, ParamOffset});
+  ParamOffset += align(Size);
+  Args.push_back(*T);

rsmith wrote:
> What happens if this overflows (due to many large local variables)?
Overflow happens here if a function takes more than a few million arguments. I 
think clang would die before it gets here.



Comment at: clang/lib/AST/ExprVM/Compiler.cpp:488-503
+Error Compiler::discard(const Expr *DE) {
+  switch (DE->getStmtClass()) {
+  case Stmt::BinaryOperatorClass:
+return discardBinaryOperator(static_cast(DE));
+  default:
+if (auto E = Visit(DE))
+  return E;

rsmith wrote:
> I'm uncomfortable about this: it looks like you're heading towards 
> duplicating a large portion of the bytecode generation depending on whether 
> the result is used. Can we do better somehow? (Eg, in CodeGen, at least for 
> aggregates, we use the same code but track whether the destination is used or 
> not.)
I'd like to do this in a future diff, once we add support for more constructs 
which discard their results.



Comment at: clang/lib/AST/ExprVM/InterpFrame.h:89-90
+  char *Args = nullptr;
+  /// Fixed, initial storage for known local variables.
+  std::unique_ptr Locals;
+};

rsmith wrote:
> Separate allocation for each frame seems wasteful. Have you considered using 
> a slab-allocated stack or similar? (Eg, allocate a large block up front and 
> allocate the locals from that, and allocate more blocks if the first one is 
> exhausted; you can cache the blocks on the `vm::Context` for reuse with no 
> extra round-trips to the heap.)
Yes, I considered allocating this on the stack - I just don't want to add that 
complexity to the interpreter yet. I like to avoid changing this until it 
becomes a noticeable performance problem.



Comment at: clang/lib/AST/ExprVM/Pointer.h:30
+
+/// Describes a memory block - generated once by the compiler.
+struct Descriptor {

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.



Comment at: clang/lib/AST/ExprVM/Pointer.h:36-39
+  /// Size of the object.
+  unsigned Size;
+  /// Size of an element.
+  unsigned ElemSize;

rsmith wrote:
> Please use a distinct type for representing size-in-the-interpreter, to avoid 
> confusion and bugs when dealing with code that must manage both compile-time 
> sizes and runtime sizes. And please use `CharUnits` for representing 
> sizes-at-runtime.
I've added an alias to be used for the sizes of object as determined by the 
interpreter - CharUnits will be used when interfacing with APValue.



Comment at: clang/lib/AST/ExprVM/Pointer.h:40-49
+  /// Type of the elements.
+  PrimType ElemType;
+  /// Flag indicating if the descriptor is mutable.
+  bool IsMutable = false;
+  /// Flag indicating if the object is an array.
+  bool IsArray = false;
+  /// Flag indicating if the object is a global.

rsmith wrote:
> Consider packing these 5 members into 4 bytes. 
I'd like to avoid packing stuff for now, makes it harder to change things later 
and it's not a performance problem yet.



Comment at: clang/lib/AST/ExprVM/Program.h:105
+  /// Program bytecode.
+  std::vector Code;
+  /// Mapping from bytecode offsets to source locations.

rsmith wrote:
> If `CodePtr` is a plain pointer, using a single flat vector here seems 
> dangerous; you'll have invalidation issues if you ever trigger creation of a 
> `Function` while evaluating, which seems like something that will happen in 
> practice, eg:
> 
> ```
> constexpr int f();
> constexpr int g() { return f(); }
> constexpr int f() { return 0; } // or imported from a module at this point
> constexpr int k = g(); // evaluation of this call triggers generation of code 
> for f
> ```
This wouldn't have been a problem, but the compilation of default constructors 
caused issues. Now the compiler can recursively invoke itself.



Comment at: clang/lib/AST/ExprVM/Program.h:106-107
+

[PATCH] D64146: [ConstExprPreter][WIP] Initial patch for the constexpr interpreter

[Nandor Licker via Phabricator via cfe-commits]

nand marked 10 inline comments as done.
nand added a comment.

We can add a separate integer type which tracks all the additional information 
required by `__builtin_constant_p` and compile all integers to it in this 
context. A later patch added an APInt fallback to the interpreter if an 
integral cannot be mapped to a type supported by the VM - this mechanism could 
be used to implement the fallback for contexts which cast pointers to integers.




Comment at: clang/lib/AST/ExprVM/Compiler.h:125
+/// Size of the local, in bytes.
+unsigned Size;
+  };

jfb wrote:
> `ByteSize` since it's the size in bytes :)
I've removed the size field since it's not going to be used, but left the 
structure since it will gain other fields in future patches.


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

[PATCH] D64146: [ConstExprPreter][WIP] Initial patch for the constexpr interpreter

[Richard Smith - zygoloid via Phabricator via cfe-commits]

rsmith added a comment.

In D64146#1604717 , @nand wrote:

> > How do you intend to represent pointers cast to integer types? Allocating 
> > 64 bits of state for a 64-bit integer is insufficient to model that case.
>
> Is this ever going to be allowed in constexpr?


It's not sufficient for this to handle only the things that are allowed in 
constant expressions; you also need to allow the things that are allowed by 
Clang's current constant evaluator, which includes this case. There are also 
constructs that allow arbitrary constant folding within the context of constant 
expression evaluation (such as a `__builtin_constant_p` conditional). So yes, 
you need to deal with this.

> If that is the case, we'll add a separate type for all integers which are 
> large enough to hold a pointer, a tagged union indicating whether the value 
> is a number or a pointer. This would add significant overhead, but I don't 
> see any other way which can correctly diagnose UB when casting a random 
> integer to a pointer.

These cases are likely to be rare enough that separately-allocated storage for 
this case could work. You'll need at least one bit somewhere to track whether 
you're in the "pointer cast to integer" case, though.

(You also need to be able to distinguish between an integer value and an 
uninitialized integer and an out-of-lifetime integer, so you'll presumably need 
some side-table to track the state of subobjects anyway.)


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

[PATCH] D64146: [ConstExprPreter][WIP] Initial patch for the constexpr interpreter

[JF Bastien via Phabricator via cfe-commits]

jfb added a comment.

In D64146#1604717 , @nand wrote:

> > How do you intend to represent pointers cast to integer types? Allocating 
> > 64 bits of state for a 64-bit integer is insufficient to model that case.
>
> Is this ever going to be allowed in constexpr? If that is the case, we'll add 
> a separate type for all integers which are large enough to hold a pointer, a 
> tagged union indicating whether the value is a number or a pointer. This 
> would add significant overhead, but I don't see any other way which can 
> correctly diagnose UB when casting a random integer to a pointer.


Most integers won't be in that category, you can therefore speculate that fact 
when emitting bytecode, and throw away byte code when the assumption turns out 
to be wrong (and re-generate the more expensive byte code).


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

[PATCH] D64146: [ConstExprPreter][WIP] Initial patch for the constexpr interpreter

[Nandor Licker via Phabricator via cfe-commits]

nand added a comment.

> How do you intend to represent pointers cast to integer types? Allocating 64 
> bits of state for a 64-bit integer is insufficient to model that case.

Is this ever going to be allowed in constexpr? If that is the case, we'll add a 
separate type for all integers which are large enough to hold a pointer, a 
tagged union indicating whether the value is a number or a pointer. This would 
add significant overhead, but I don't see any other way which can correctly 
diagnose UB when casting a random integer to a pointer.


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

[PATCH] D64146: [ConstExprPreter][WIP] Initial patch for the constexpr interpreter

[Richard Smith - zygoloid via Phabricator via cfe-commits]

rsmith added a comment.

How do you intend to represent pointers cast to integer types? Allocating 64 
bits of state for a 64-bit integer is insufficient to model that case.




Comment at: clang/include/clang/AST/ASTContext.h:583-584
 
+  /// Returns the constexpr VM context.
+  vm::Context &getConstexprCtx();
+

You have too many names for the same thing (`vm`, `ExprVM`, ConstExprPreter, 
and `ConstexprCtx` here -- and `ExprVM` is a bad choice of name since a lot of 
the purpose is to evaluate statements rather than expressions). Please pick a 
single name and apply it consistently.

Suggestion: use namespace `clang::interp`, in `{include/clang,lib}/AST/Interp`, 
and generally refer to this implementation as "the Clang interpreter". This 
function should be called `getInterpContext` or similar. (If you prefer, use VM 
intead of Interp, but I'd be concerned about confusion with LLVM.)



Comment at: clang/include/clang/Basic/DiagnosticASTKinds.td:231-234
+def err_constexpr_compilation_failed : Error<
+  "constexpr cannot be compiled by the experimental interpreter">;
+def err_constexpr_interp_failed : Error<
+  "constexpr cannot be evaluated by the experimental interpreter">;

`constexpr` is an adjective; this sentence doesn't parse. Do you mean "this 
constexpr function" or something like that?



Comment at: clang/include/clang/Driver/Options.td:838-839
+  HelpText<"Enable the experimental constexpr interpreter">, 
Flags<[CC1Option]>;
+def fexperimental_constexpr_force_interpreter : Flag<["-"], 
"fexperimental-constexpr-force-interpreter">, Group,
+  HelpText<"Force the use of the experimental constexpr interpreter, failing 
on missing features">, Flags<[CC1Option]>;
 def fconstexpr_backtrace_limit_EQ : Joined<["-"], 
"fconstexpr-backtrace-limit=">,

If the name we're presenting to users is "the experimental constexpr 
interpreter", then "force" shouldn't appear part way through that name. 
`-fforce-experimental-constexpr-interpreter` maybe?



Comment at: clang/lib/AST/CMakeLists.txt:85
   LINK_LIBS
+  clangExpr
   clangBasic

What is the `clangExpr` library? Should this say `clangExprVM` (modulo 
renaming)?



Comment at: clang/lib/AST/ExprConstant.cpp:691-696
+/// Force the use of the constexpr interpreter, bailing out with an error
+/// if a feature is unsupported.
+bool ForceInterp;
+
+/// Enable the constexpr interpreter.
+bool EnableInterp;

These comments don't make sense: `ExprConstant.cpp` is a constexpr interpreter 
too. Maybe "the new interpreter"?



Comment at: clang/lib/AST/ExprVM/CMakeLists.txt:4
+
+add_clang_library(clangExpr
+  Compiler.cpp

This is not a reasonable name for this library.



Comment at: clang/lib/AST/ExprVM/Compiler.cpp:79-82
+  for (auto *ParamDecl : F->parameters()) {
+if (auto T = Ctx.classify(ParamDecl->getType())) {
+  auto Size = primSize(*T);
+  auto Desc = llvm::make_unique(ParamDecl, Size, Size, *T,

You are overusing `auto` here. The types of at least `T` and `Size` are 
non-obvious, and should be written explicitly.



Comment at: clang/lib/AST/ExprVM/Compiler.cpp:88
+  Params.insert({ParamDecl, ParamOffset});
+  ParamOffset += align(Size);
+  Args.push_back(*T);

What happens if this overflows (due to many large local variables)?



Comment at: clang/lib/AST/ExprVM/Compiler.cpp:99
+Start = P.getOffset();
+if (auto E = compile(F->getBody()))
+  return std::move(E);

Don't use `auto` here. The code would be much clearer if you explicitly write 
`llvm::Error`.

(I'm not going to call out further overuse of `auto`; generally [the rule we 
want to 
follow](http://llvm.org/docs/CodingStandards.html#use-auto-type-deduction-to-make-code-more-readable)
 is "Use auto if and only if it makes the code more readable or easier to 
maintain.", which many of the uses of `auto` here do not.)



Comment at: clang/lib/AST/ExprVM/Compiler.cpp:144
+  } else {
+return discard(static_cast(S));
+  }

Use `E` here rather than casting `S` again.



Comment at: clang/lib/AST/ExprVM/Compiler.cpp:168-169
+  if (!VD->hasLocalStorage()) {
+// TODO: implement non-local variables.
+return bail(DS);
+  }

Non-local variables should not require any code generation, since they can't 
have dynamic initialization or destruction. (You can just return 
`Error::success()`.)



Comment at: clang/lib/AST/ExprVM/Compiler.cpp:321-339
+if (auto T = Ctx.classify(CE->getType())) {
+  if (auto *DE = dyn_cast(SubExpr)) {
+bool IsReference = DE->getDecl()->getType()->isReferenceType();
+if (!IsReference) {
+  if (auto *P

[PATCH] D64146: [ConstExprPreter][WIP] Initial patch for the constexpr interpreter

[JF Bastien via Phabricator via cfe-commits]

jfb added inline comments.



Comment at: clang/lib/AST/ExprVM/Compiler.cpp:85
+/* isArray */ false,
+/* isGlobal */ false);
+  ParamDescriptors.insert({ParamOffset, std::move(Desc)});

Usually the style used is `/*isMutable=*/true`



Comment at: clang/lib/AST/ExprVM/Compiler.h:69
+public:
+  /// Creates a compiler for a funciton.
+  Compiler(Context &Ctx, Program &P, const FunctionDecl *F);

"function"



Comment at: clang/lib/AST/ExprVM/Compiler.h:125
+/// Size of the local, in bytes.
+unsigned Size;
+  };

`ByteSize` since it's the size in bytes :)


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