[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

803

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

1154

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

282

Why not hasGlobalStorage()?

Also what is the logic behind isConstexpr() check?

[tbaeder]

clang/lib/AST/Interp/ByteCodeExprGen.cpp

803

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

clang/lib/AST/Interp/ByteCodeExprGen.h

282

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

282

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

282

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

282

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

[tbaeder]

clang/lib/AST/Interp/ByteCodeExprGen.h

282

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

803

822

1127–1130

and if we have no GlobalIndex?

Should this instead be:

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

1129–1131

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

1161

What if Init is nullptr?

1165

Same here?

clang/lib/AST/Interp/ByteCodeExprGen.h

96

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?

282

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

1127–1130

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

1161

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

[aaron.ballman]

clang/lib/AST/Interp/ByteCodeExprGen.cpp

1127–1130

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

1161

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

262

[tbaeder]

clang/lib/AST/Interp/ByteCodeExprGen.cpp

1127–1130

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

1127–1130

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

1127–1130

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

1125

I don't get the comment.

[tbaeder]

clang/lib/AST/Interp/ByteCodeExprGen.cpp

1125

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 VisitCharacterLiteral(const CharacterLiteral *E);
   bool VisitExprWithCleanups(const ExprWithCleanups *E);
   bool VisitMaterializeTemporaryExpr(const MaterializeTemporaryExpr *E);
   bool VisitCompoundAssignOperator(const CompoundAssignOperator *E);
 
 protected:
   bool visitExpr(const Expr *E) override;
   bool visitDecl(const VarDecl *VD) override;
+  bool visitVarDecl(const VarDecl *VD);
 
-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.
   Record *getRecord(QualType Ty);
const CXXRecordDecl *getRecordDecl(const Expr *E) const {
     QualType T = E->getType();
     if (const auto *RD = T->getPointeeCXXRecordDecl())
       return RD;
     return T->getAsCXXRecordDecl();
   }
 
 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.
   llvm::Optional<uint64_t> ArrayIndex;
 
   /// Flag indicating if return value is to be discarded.
   bool DiscardResult = false;
 
   /// Expression being initialized.
   llvm::Optional<InitFnRef> InitFn = {};
+
+  /// Returns whether we should create a global variable for the
+  /// given VarDecl.
+  bool isGlobalDecl(const VarDecl *VD) const {
+    return !VD->hasLocalStorage() || VD->isConstexpr();
+  }
 };
 
 extern template class ByteCodeExprGen<ByteCodeEmitter>;
 extern template class ByteCodeExprGen<EvalEmitter>;
 
 /// Scope chain managing the variable lifetimes.
 template <class Emitter> class VariableScope {
 public:

clang/lib/AST/Interp/ByteCodeExprGen.cpp

bool ByteCodeExprGen<Emitter>::emitConst(PrimType T, const APInt &Value,
   llvm_unreachable("unknown primitive type");
 }
 
 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 Decl *VD = Src.dyn_cast<const Decl *>(); VD && isa<ValueDecl>(VD)) {
+    assert(!P.getGlobal(cast<ValueDecl>(VD)));
+    assert(Locals.find(cast<ValueDecl>(VD)) == Locals.end());
+  }
+
   const Descriptor::MetadataSize MDSize{sizeof(InlineDescriptor)};
   Descriptor *D =
       P.createDescriptor(Src, Ty, MDSize, 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>
 llvm::Optional<unsigned>
 ByteCodeExprGen<Emitter>::allocateLocal(DeclTy &&Src, bool IsExtended) {
+  // Make sure we don't accidentally register the same decl twice.
+  if (const Decl *VD = Src.dyn_cast<const Decl *>(); VD && isa<ValueDecl>(VD)) {
+    assert(!P.getGlobal(cast<ValueDecl>(VD)));
+    assert(Locals.find(cast<ValueDecl>(VD)) == Locals.end());
+  }
+
   const Descriptor::MetadataSize MDSize{sizeof(InlineDescriptor)};
   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;
if (!visit(Exp))
     return false;
 
   if (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();
+  Optional<PrimType> VarT = classify(VD->getType());
 
-  if (Optional<unsigned> I = P.createGlobal(VD, Init)) {
-    if (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))
+  // Get a pointer to the variable
+  if (isGlobalDecl(VD)) {
+    if (auto GlobalIndex = P.getGlobal(VD)) {
+      if (!this->emitGetPtrGlobal(*GlobalIndex, 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))
+    }
+    if (auto It = Locals.find(VD); It != Locals.end()) {
+      if (!this->emitGetPtrLocal(It->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();
+  Optional<PrimType> VarT = classify(VD->getType());
+
+  if (isGlobalDecl(VD)) {
+    Optional<unsigned> GlobalIndex = P.getOrCreateGlobal(VD, Init);
+
+    if (!GlobalIndex)
+      return this->bail(VD);
+
+
+      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);
+      }
+      return true;
+    } else {
+      if (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)) {

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.
   llvm::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 (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 (Optional<unsigned> Offset = this->allocateLocal(VD))
-    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.
   llvm::Optional<unsigned> getGlobal(const ValueDecl *VD);
 
   /// Returns or creates a global an creates an index to it.
-  llvm::Optional<unsigned> getOrCreateGlobal(const ValueDecl *VD);
+  llvm::Optional<unsigned> getOrCreateGlobal(const ValueDecl *VD,
+                                             const Expr *Init = nullptr);
 
   /// Returns or creates a dummy value for parameters.
   llvm::Optional<unsigned> getOrCreateDummy(const ParmVarDecl *PD);
 
   /// Creates a global and returns its index.
   llvm::Optional<unsigned> createGlobal(const ValueDecl *VD, const Expr *E);
 
   /// Creates a global from a lifetime-extended temporary.

clang/lib/AST/Interp/Program.cpp

llvm::Optional<unsigned> Program::getGlobal(const ValueDecl *VD) {
   if (Index) {
     GlobalIndices[VD] = *Index;
     return {};
   }
 
   return Index;
 }
 
-llvm::Optional<unsigned> Program::getOrCreateGlobal(const ValueDecl *VD) {
+llvm::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 {};
 }
 
 llvm::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 {};
 }
 
 llvm::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;
   }