[clang][Interp] Unify visiting variable declarations

[tbaeder]

Summary

We often visit the same variable multiple times, e.g. once when checking
its initializer and later when compiling the function. Unify both of
those in visitVarDecl() and do the returning of the value in
visitDecl().

I was working on something different and noticed that we register the same variable declaration twice.

Clang calls EvaluateAsInitializer() for local (const, probably) declarations as well, so we shouldn't register those as global variables.

[shafik]

clang/lib/AST/Interp/ByteCodeExprGen.cpp

806

Nitpick, I find using VD a bit confusing since we don't know if it is a ValueDecl until after it is initialized.

1237

I wonder if the code in both branches might be better factored out into their own functions and perhaps move the DeclScope<Emitter> LocalScope(this, VD); out.

clang/lib/AST/Interp/ByteCodeExprGen.h

267

Why not hasGlobalStorage()?

Also what is the logic behind isConstexpr() check?

[tbaeder]

clang/lib/AST/Interp/ByteCodeExprGen.cpp

806

Replaced this with the same version we use a few lines below anyway.

clang/lib/AST/Interp/ByteCodeExprGen.h

267

Didn't know isGlobalStorage() existed ;)

Constexpr local variables can be handled like global ones, can't they? That was the logic behind it, nothing else. We can save ourselves the hassle of local variables in that case.

[shafik]

clang/lib/AST/Interp/ByteCodeExprGen.h

267

I think I am missing a level of logic here. I don't think of constant expressions as needing storage nor do I think of them as global variables.

So can you take a step back and explain how this fits in the bigger picture?

[tbaeder]

clang/lib/AST/Interp/ByteCodeExprGen.h

267

They don't necessarily need storage in the final executable but we create global/local variables for them for bookkeeping, e.g. we need to be able to take their address, track the value, etc.

[shafik]

clang/lib/AST/Interp/ByteCodeExprGen.h

267

Ok, will this work for recursive constexpr functions with local variables?

[tbaeder]

clang/lib/AST/Interp/ByteCodeExprGen.h

267

local constexpr variables still have to be initialized and are immutable, so that doesn't make a difference for recursive functions since none of the recursive calls can change the variable value. I did a small local test but since the variable can't be modified, it's not very interesting.

[tbaeder]

Ping

[tbaeder]

Ping

[tbaeder]

Ping

[aaron.ballman]

clang/lib/AST/Interp/ByteCodeExprGen.cpp

806

828

1212–1213

and if we have no GlobalIndex?

Should this instead be:

if (auto GlobalIndex = P.getGlobal(VD); !GlobalIndex || !this->emitGetPtrGlobal(*GlobalIndex, VD))
  return false;

1214–1216

Similar question here as above -- what if It == Locals.end()?

1220

Same here?

1244

What if Init is nullptr?

clang/lib/AST/Interp/ByteCodeExprGen.h

93

I was a bit surprised to not see override here -- I wonder if we should have some visual separation between the overrides and the non-overrides given the similarity in naming?

267

I find the interface to be confusing. If I want to know "is this a global decl", I am not going to expect a local constexpr variable to go "oh yeah, I totally am!". Perhaps this should be renamed to something more about the semantics, like shouldBeGloballyIndexed() or something along those lines?

[tbaeder]

clang/lib/AST/Interp/ByteCodeExprGen.cpp

1212–1213

I mean, that shouldn't happen because the visitVarDecl call before would've returned false in that case.

1244

Global variables (or local constexpr ones) must have an initializer, no?

[aaron.ballman]

clang/lib/AST/Interp/ByteCodeExprGen.cpp

1212–1213

The use of an if makes it seem like it could fail. Perhaps we should drop the if and use an assert?

1244

Hmm, I was thinking of:

struct S {};
constexpr S s;

but now that I think on it, that still has a non-null init expression, so I think it's safe for us to assert Init is nonnull.

clang/lib/AST/Interp/ByteCodeExprGen.h

241

[tbaeder]

clang/lib/AST/Interp/ByteCodeExprGen.cpp

1212–1213

What do you think about returning a tuple(valid, offset) from visitVarDecl? Although that might make the call in ByteCodeStmtGen.cpp a little awkward.

[aaron.ballman]

clang/lib/AST/Interp/ByteCodeExprGen.cpp

1212–1213

Hmmmm, I don't think I'd like to see a tuple, but seeing an llvm::ErrorOr<> or a std::optional<> might be reasonable. The downside is that the visit* functions then won't have a uniform return type (can't do ErrorOr<void> that I'm aware of)

[erichkeane]

clang/lib/AST/Interp/ByteCodeExprGen.cpp

1212–1213

The way I've seen in the past is to have it be an error-type containing a nullptr_t (or some sort of empty/meaningless struct).

So something like:

ErrorOr<Void>

[tbaeder]

Meh, just use an assert :|

[aaron.ballman]

LGTM!

[shafik]

LGTM

clang/lib/AST/Interp/ByteCodeExprGen.cpp

1158

I don't get the comment.

[tbaeder]

clang/lib/AST/Interp/ByteCodeExprGen.cpp

1158

P.getGlobal() must return a global index and not std::nullopt, since the visitVarDecl() call above did not return false. It will return false if something failed, but it didn't and so the variable must be allocated already.

clang/lib/AST/Interp/ByteCodeExprGen.h

public:
   bool VisitAbstractConditionalOperator(const AbstractConditionalOperator *E);
   bool VisitStringLiteral(const StringLiteral *E);
   bool VisitCharacterLiteral(const CharacterLiteral *E);
   bool VisitCompoundAssignOperator(const CompoundAssignOperator *E);
 
 protected:
   bool visitExpr(const Expr *E) override;
   bool visitDecl(const VarDecl *VD) override;
 
 protected:
   /// Emits scope cleanup instructions.
   void emitCleanup();
 
   /// Returns a record type from a record or pointer type.
   const RecordType *getRecordTy(QualType Ty);
 
   /// Returns a record from a record or pointer type.
protected:
   /// Evaluates an expression and places result on stack.
   bool visit(const Expr *E);
   /// Compiles an initializer.
   bool visitInitializer(const Expr *E);
   /// Compiles an array initializer.
   bool visitArrayInitializer(const Expr *Initializer);
   /// Compiles a record initializer.
   bool visitRecordInitializer(const Expr *Initializer);
+  /// Creates and initializes a variable from the given decl.
+  bool visitVarDecl(const VarDecl *VD);
 
   /// Visits an expression and converts it to a boolean.
   bool visitBool(const Expr *E);
 
   /// Visits an initializer for a local.
   bool visitLocalInitializer(const Expr *Init, unsigned I) {
     if (!this->emitGetPtrLocal(I, Init))
       return false;
private:
   /// or nullptr if no such decl exists.
   const CXXRecordDecl *getRecordDecl(const Expr *E) const {
     QualType T = E->getType();
     if (const auto *RD = T->getPointeeCXXRecordDecl())
       return RD;
     return T->getAsCXXRecordDecl();
   }
 
+  /// Returns whether we should create a global variable for the
+  /// given VarDecl.
+  bool shouldBeGloballyIndexed(const VarDecl *VD) const {
+    return VD->hasGlobalStorage() || VD->isConstexpr();
+  }
+
 protected:
   /// Variable to storage mapping.
   llvm::DenseMap<const ValueDecl *, Scope::Local> Locals;
 
   /// OpaqueValueExpr to location mapping.
   llvm::DenseMap<const OpaqueValueExpr *, unsigned> OpaqueExprs;
 
   /// Current scope.
   VariableScope<Emitter> *VarScope = nullptr;
 
   /// Current argument index. Needed to emit ArrayInitIndexExpr.
   std::optional<uint64_t> ArrayIndex;
 
   /// Flag indicating if return value is to be discarded.
   bool DiscardResult = false;
 
   /// Expression being initialized.
   std::optional<InitFnRef> InitFn = {};
 };
 
 extern template class ByteCodeExprGen<ByteCodeEmitter>;
 extern template class ByteCodeExprGen<EvalEmitter>;
 
 /// Scope chain managing the variable lifetimes.
 template <class Emitter> class VariableScope {
 public:
   virtual ~VariableScope() { Ctx->VarScope = this->Parent; }
 
   void add(const Scope::Local &Local, bool IsExtended) {
     if (IsExtended)
       this->addExtended(Local);

clang/lib/AST/Interp/ByteCodeExprGen.cpp

bool ByteCodeExprGen<Emitter>::emitConst(const APSInt &Value, const Expr *E) {
   return this->emitConst(Value.getZExtValue(), E);
 }
 
 template <class Emitter>
 unsigned ByteCodeExprGen<Emitter>::allocateLocalPrimitive(DeclTy &&Src,
                                                           PrimType Ty,
                                                           bool IsConst,
                                                           bool IsExtended) {
+  // Make sure we don't accidentally register the same decl twice.
+  if (const auto *VD =
+          dyn_cast_if_present<ValueDecl>(Src.dyn_cast<const Decl *>())) {
+    assert(!P.getGlobal(VD));
+    assert(Locals.find(VD) == Locals.end());
+  }
+
   // FIXME: There are cases where Src.is<Expr*>() is wrong, e.g.
   //   (int){12} in C. Consider using Expr::isTemporaryObject() instead
   //   or isa<MaterializeTemporaryExpr>().
   Descriptor *D = P.createDescriptor(Src, Ty, Descriptor::InlineDescMD, IsConst,
                                      Src.is<const Expr *>());
   Scope::Local Local = this->createLocal(D);
   if (auto *VD = dyn_cast_or_null<ValueDecl>(Src.dyn_cast<const Decl *>()))
     Locals.insert({VD, Local});
   VarScope->add(Local, IsExtended);
   return Local.Offset;
 }
 
 template <class Emitter>
 std::optional<unsigned>
 ByteCodeExprGen<Emitter>::allocateLocal(DeclTy &&Src, bool IsExtended) {
-  QualType Ty;
+  // Make sure we don't accidentally register the same decl twice.
+  if (const auto *VD =
+          dyn_cast_if_present<ValueDecl>(Src.dyn_cast<const Decl *>())) {
+    assert(!P.getGlobal(VD));
+    assert(Locals.find(VD) == Locals.end());
+  }
 
+  QualType Ty;
   const ValueDecl *Key = nullptr;
   const Expr *Init = nullptr;
   bool IsTemporary = false;
   if (auto *VD = dyn_cast_if_present<ValueDecl>(Src.dyn_cast<const Decl *>())) {
     Key = VD;
     Ty = VD->getType();
 
     if (const auto *VarD = dyn_cast<VarDecl>(VD))
if (!visit(Exp))
     return false;
 
   if (std::optional<PrimType> T = classify(Exp))
     return this->emitRet(*T, Exp);
   else
     return this->emitRetValue(Exp);
 }
 
+/// Toplevel visitDecl().
+/// We get here from evaluateAsInitializer().
+/// We need to evaluate the initializer and return its value.
 template <class Emitter>
 bool ByteCodeExprGen<Emitter>::visitDecl(const VarDecl *VD) {
-  const Expr *Init = VD->getInit();
+  std::optional<PrimType> VarT = classify(VD->getType());
 
-  if (std::optional<unsigned> I = P.createGlobal(VD, Init)) {
-    if (std::optional<PrimType> T = classify(VD->getType())) {
-      {
-        // Primitive declarations - compute the value and set it.
-        DeclScope<Emitter> LocalScope(this, VD);
-        if (!visit(Init))
-          return false;
-      }
+  // Create and initialize the variable.
+  if (!this->visitVarDecl(VD))
 
-      // If the declaration is global, save the value for later use.
-      if (!this->emitDup(*T, VD))
-        return false;
-      if (!this->emitInitGlobal(*T, *I, VD))
-        return false;
-      return this->emitRet(*T, VD);
-    } else {
-      {
-        // Composite declarations - allocate storage and initialize it.
-        DeclScope<Emitter> LocalScope(this, VD);
-        if (!visitGlobalInitializer(Init, *I))
+  // Get a pointer to the variable
+  if (shouldBeGloballyIndexed(VD)) {
+    auto GlobalIndex = P.getGlobal(VD);
+    assert(GlobalIndex); // visitVarDecl() didn't return false.
+    if (!this->emitGetPtrGlobal(*GlobalIndex, VD))
+    auto Local = Locals.find(VD);
+    assert(Local != Locals.end()); // Same here.
+    if (!this->emitGetPtrLocal(Local->second.Offset, VD))
+      return false;
+  }
+
+  // Return the value
+  if (VarT) {
+    if (!this->emitLoadPop(*VarT, VD))
+      return false;
+
+    return this->emitRet(*VarT, VD);
+  }
+
+  return this->emitRetValue(VD);
+}
+
+template <class Emitter>
+bool ByteCodeExprGen<Emitter>::visitVarDecl(const VarDecl *VD) {
+  const Expr *Init = VD->getInit();
+  std::optional<PrimType> VarT = classify(VD->getType());
+
+  if (shouldBeGloballyIndexed(VD)) {
+    std::optional<unsigned> GlobalIndex = P.getOrCreateGlobal(VD, Init);
+
+    if (!GlobalIndex)
+      return this->bail(VD);
+
+    assert(Init);
+
+      if (VarT) {
+        if (!this->visit(Init))
           return false;
+        return this->emitInitGlobal(*VarT, *GlobalIndex, VD);
       }
+      return this->visitGlobalInitializer(Init, *GlobalIndex);
+    }
+  } else {
+    DeclScope<Emitter> LocalScope(this, VD);
 
-      // Return a pointer to the global.
-      if (!this->emitGetPtrGlobal(*I, VD))
+    if (VarT) {
+      unsigned Offset = this->allocateLocalPrimitive(
+          VD, *VarT, VD->getType().isConstQualified());
+      // Compile the initializer in its own scope.
+      if (Init) {
+        ExprScope<Emitter> Scope(this);
+        if (!this->visit(Init))
           return false;
-      return this->emitRetValue(VD);
+
+        return this->emitSetLocal(*VarT, Offset, VD);
+      }
+    } else {
+      if (std::optional<unsigned> Offset = this->allocateLocal(VD)) {
+        if (Init)
+          return this->visitLocalInitializer(Init, *Offset);
+      }
     }
+    return true;
   }
 
-  return this->bail(VD);
+  return false;
 }
 
 template <class Emitter>
 bool ByteCodeExprGen<Emitter>::VisitCallExpr(const CallExpr *E) {
   assert(!E->getBuiltinCallee() && "Builtin functions aren't supported yet");
 
   const Decl *Callee = E->getCalleeDecl();
   if (const auto *FuncDecl = dyn_cast_or_null<FunctionDecl>(Callee)) {
     const Function *Func = getFunction(FuncDecl);
     if (!Func)
       return false;
     // If the function is being compiled right now, this is a recursive call.
     // In that case, the function can't be valid yet, even though it will be
     // later.
     // If the function is already fully compiled but not constexpr, it was
     // found to be faulty earlier on, so bail out.
     if (Func->isFullyCompiled() && !Func->isConstexpr())
       return false;
 
     QualType ReturnType = E->getCallReturnType(Ctx.getASTContext());
     std::optional<PrimType> T = classify(ReturnType);
 
     if (Func->hasRVO() && DiscardResult) {
       // If we need to discard the return value but the function returns its
       // value via an RVO pointer, we need to create one such pointer just
       // for this call.
       if (std::optional<unsigned> LocalIndex = allocateLocal(E)) {
         if (!this->emitGetPtrLocal(*LocalIndex, E))

clang/lib/AST/Interp/ByteCodeStmtGen.h

private:
   bool visitIfStmt(const IfStmt *IS);
   bool visitWhileStmt(const WhileStmt *S);
   bool visitDoStmt(const DoStmt *S);
   bool visitForStmt(const ForStmt *S);
   bool visitBreakStmt(const BreakStmt *S);
   bool visitContinueStmt(const ContinueStmt *S);
 
   /// Compiles a variable declaration.
-  bool visitVarDecl(const VarDecl *VD);
 
 private:
   /// Type of the expression returned by the function.
   std::optional<PrimType> ReturnType;
 
   /// Switch case mapping.
   CaseMap CaseLabels;
 

clang/lib/AST/Interp/ByteCodeStmtGen.cpp

bool ByteCodeStmtGen<Emitter>::visitCompoundStmt(
   return true;
 }
 
 template <class Emitter>
 bool ByteCodeStmtGen<Emitter>::visitDeclStmt(const DeclStmt *DS) {
   for (auto *D : DS->decls()) {
     // Variable declarator.
     if (auto *VD = dyn_cast<VarDecl>(D)) {
-      if (!visitVarDecl(VD))
+      if (!this->visitVarDecl(VD))
         return false;
       continue;
     }
 
     // Decomposition declarator.
     if (auto *DD = dyn_cast<DecompositionDecl>(D)) {
       return this->bail(DD);
     }

 template <class Emitter>
 bool ByteCodeStmtGen<Emitter>::visitContinueStmt(const ContinueStmt *S) {
   if (!ContinueLabel)
     return false;
 
   return this->jump(*ContinueLabel);
 }
 
-template <class Emitter>
-bool ByteCodeStmtGen<Emitter>::visitVarDecl(const VarDecl *VD) {
-  if (!VD->hasLocalStorage()) {
-    // No code generation required.
-    return true;
-  }
-
-  // Integers, pointers, primitives.
-  if (std::optional<PrimType> T = this->classify(VD->getType())) {
-    const Expr *Init = VD->getInit();
-
-    unsigned Offset =
-        this->allocateLocalPrimitive(VD, *T, VD->getType().isConstQualified());
-    // Compile the initializer in its own scope.
-    if (Init) {
-      ExprScope<Emitter> Scope(this);
-      if (!this->visit(Init))
-        return false;
-
-      return this->emitSetLocal(*T, Offset, VD);
-    }
-    return true;
-  }
-
-  // Composite types - allocate storage and initialize it.
-  if (std::optional<unsigned> Offset = this->allocateLocal(VD)) {
-    if (!VD->getInit())
-      return true;
-
-    return this->visitLocalInitializer(VD->getInit(), *Offset);
-  }
-
-  return this->bail(VD);
-}
-
 namespace clang {
 namespace interp {
 
 template class ByteCodeStmtGen<ByteCodeEmitter>;
 
 } // namespace interp
 } // namespace clang

clang/lib/AST/Interp/Program.h

Block *getGlobal(unsigned Idx) {
     assert(Idx < Globals.size());
     return Globals[Idx]->block();
   }
 
   /// Finds a global's index.
   std::optional<unsigned> getGlobal(const ValueDecl *VD);
 
   /// Returns or creates a global an creates an index to it.
-  std::optional<unsigned> getOrCreateGlobal(const ValueDecl *VD);
+  std::optional<unsigned> getOrCreateGlobal(const ValueDecl *VD,
+                                            const Expr *Init = nullptr);
 
   /// Returns or creates a dummy value for parameters.
   std::optional<unsigned> getOrCreateDummy(const ParmVarDecl *PD);
 
   /// Creates a global and returns its index.
   std::optional<unsigned> createGlobal(const ValueDecl *VD, const Expr *E);
 
   /// Creates a global from a lifetime-extended temporary.

clang/lib/AST/Interp/Program.cpp

std::optional<unsigned> Program::getGlobal(const ValueDecl *VD) {
   if (Index) {
     GlobalIndices[VD] = *Index;
     return {};
   }
 
   return Index;
 }
 
-std::optional<unsigned> Program::getOrCreateGlobal(const ValueDecl *VD) {
+std::optional<unsigned> Program::getOrCreateGlobal(const ValueDecl *VD,
+                                                   const Expr *Init) {
   if (auto Idx = getGlobal(VD))
     return Idx;
 
-  if (auto Idx = createGlobal(VD, nullptr)) {
+  if (auto Idx = createGlobal(VD, Init)) {
     GlobalIndices[VD] = *Idx;
     return Idx;
   }
   return {};
 }
 
 std::optional<unsigned> Program::getOrCreateDummy(const ParmVarDecl *PD) {
   auto &ASTCtx = Ctx.getASTContext();
if (auto Idx = createGlobal(PD, Ty, /*isStatic=*/true, /*isExtern=*/true)) {
     DummyParams[PD] = *Idx;
     return Idx;
   }
   return {};
 }
 
 std::optional<unsigned> Program::createGlobal(const ValueDecl *VD,
                                               const Expr *Init) {
+  assert(!getGlobal(VD));
   bool IsStatic, IsExtern;
   if (auto *Var = dyn_cast<VarDecl>(VD)) {
     IsStatic = !Var->hasLocalStorage();
     IsExtern = !Var->getAnyInitializer();
   } else {
     IsStatic = false;
     IsExtern = true;
   }