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