[PATCH] D64146: [Clang Interpreter] Initial patch for the constexpr interpreter

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

rsmith added a comment.

I think we're past the point of large-scale structural comments that are better addressed before the first commit, and it'd be much better to make incremental improvements from here.

Please go ahead and commit this, and we can iterate in-tree from now on. Thanks!



================
Comment at: clang/include/clang/Basic/OptionalDiagnostic.h:40-74
+  OptionalDiagnostic &operator<<(const llvm::APSInt &I) {
+    if (Diag) {
+      SmallVector<char, 32> Buffer;
+      I.toString(Buffer);
+      *Diag << StringRef(Buffer.data(), Buffer.size());
+    }
+    return *this;
----------------
I think these three all belong in `PartialDiagnostic` rather than in `OptionalDiagnostic`. (It doesn't make sense that an `OptionalDiagnostic` can format an `APSInt` but a `PartialDiagnostic` cannot.)


================
Comment at: clang/include/clang/Driver/Options.td:839
+def fforce_experimental_new_constant_interpreter : Flag<["-"], "fforce-experimental-new-constant-interpreter">, Group<f_Group>,
+  HelpText<"Force the use of the experimental new const interpreter, failing on missing features">, Flags<[CC1Option]>;
 def fconstexpr_backtrace_limit_EQ : Joined<["-"], "fconstexpr-backtrace-limit=">,
----------------
const -> constant


================
Comment at: clang/lib/AST/Interp/ByteCodeGen.cpp:25
+using APSInt = llvm::APSInt;
+template <typename T> using Expected = llvm::Expected<T>;
+
----------------
`using llvm::Expected;`


================
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()) {
----------------
nand wrote:
> rsmith wrote:
> > 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.
> We absolutely need this to emit the warning:
> ```
> constexpr int callee_ptr(int *beg, int *end) {
>   const int x = 2147483647;
>   *beg = x + x; // expected-warning {{overflow in expression; result is -2 with type 'int'}}\
>                 // expected-note 3{{value 4294967294 is outside the range of representable values of type 'int'}}
>   return *beg;
> }
> ```
As noted, the evaluation should be treated as non-constant in this case. This is "evaluate for overflow" mode, in which we want to keep evaluating past non-constant operations (and merely skip sub-evaluations that we can't process because they have non-constant values). We will need a bytecode representation for "non-constant evaluation". We should use that here.


================
Comment at: clang/lib/AST/Interp/ByteCodeGen.h:40
+class ByteCodeGen : public ConstStmtVisitor<ByteCodeGen<Emitter>, bool>,
+                 public Emitter {
+  using NullaryFn = bool (ByteCodeGen::*)(const SourceInfo &);
----------------
nand wrote:
> rsmith wrote:
> > Does `Emitter` need to be a base class rather than a member?
> The entry point is through the emitter which needs to call into the code generator, things don't really work if the emitter is a field.
I think the fact that the entry point to `ByteCodeGen` is in the `Emitter` base class is the cause of this design complexity (the inheritance, virtual functions, and layering cycle between the code generator and the emitter). For example, looking at `ByteCodeEmitter`, we have:

* `ByteCodeEmitter::compileFunc` is the entry point, and is the only function that calls `compile`
* `ByteCodeEmitter::compile` is a virtual function that calls into `ByteCodeGen`
* otherwise, `ByteCodeGen` only calls into `ByteCodeEmitter` and `ByteCodeEmitter` never calls into `ByteCodeGen`

(And the same thing happens in `EvalEmitter`.) That to me suggests an opportunity to improve the layering:

* move `ByteCodeEmitter::compileFunc` out of `ByteCodeEmitter` into a standalone function that creates a `ByteCodeGen<ByteCodeEmitter>` and calls `compile` on it
* remove the virtual functions and store the `Emitter` as a member of `ByteCodeGen`

No need to do this right away.


================
Comment at: clang/lib/AST/Interp/ByteCodeGen.h:85
+
+  enum class AccessKind {
+    /// Value is read and pushed to stack.
----------------
Please can you pick a different name for this, that's more different from the `AccessKinds` enum? `AccessOp` or something like that maybe?


================
Comment at: clang/lib/AST/Interp/Opcodes.td:178-221
+// [] -> [Pointer]
+def GetPtrLocal : Opcode {
+  let Args = [ArgUint32];
+  bit HasCustomEval = 1;
+}
+// [] -> [Pointer]
+def GetPtrParam : Opcode {
----------------
Please document what the `Args` mean.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D64146/new/

https://reviews.llvm.org/D64146