Re-commit "[clang][Interp] Unify visiting variable declarations"

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().

This time, use a VariableScope instead of a DeclScope for local
variables. This way, we don't emit Destroy ops for the local variables
immediately after creating them.

Differential Revision: https://reviews.llvm.org/D136815

diff --git a/clang/lib/AST/Interp/ByteCodeExprGen.cpp b/clang/lib/AST/Interp/ByteCodeExprGen.cpp
index f4bf162..4bab289 100644 (file)
--- a/clang/lib/AST/Interp/ByteCodeExprGen.cpp
+++ b/clang/lib/AST/Interp/ByteCodeExprGen.cpp
@@ -802,6 +802,13 @@ 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>().
@@ -817,8 +824,14 @@ unsigned ByteCodeExprGen<Emitter>::allocateLocalPrimitive(DeclTy &&Src,
 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;
@@ -1128,41 +1141,86 @@ bool ByteCodeExprGen<Emitter>::visitExpr(const Expr *Exp) {
     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) {
+  std::optional<PrimType> VarT = classify(VD->getType());
+
+  // Create and initialize the variable.
+  if (!this->visitVarDecl(VD))
+    return false;
+
+  // 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))
+      return false;
+  } else {
+    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);
+    {
+      DeclScope<Emitter> LocalScope(this, VD);
 
-  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))
+      if (VarT) {
+        if (!this->visit(Init))
           return false;
+        return this->emitInitGlobal(*VarT, *GlobalIndex, VD);
       }
+      return this->visitGlobalInitializer(Init, *GlobalIndex);
+    }
+  } else {
+    VariableScope<Emitter> LocalScope(this);
+    if (VarT) {
+      unsigned Offset = this->allocateLocalPrimitive(
+          VD, *VarT, VD->getType().isConstQualified());
+      if (Init) {
+        // Compile the initializer in its own scope.
+        ExprScope<Emitter> Scope(this);
+        if (!this->visit(Init))
+          return false;
 
-      // 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);
+        return this->emitSetLocal(*VarT, Offset, VD);
+      }
     } else {
-      {
-        // Composite declarations - allocate storage and initialize it.
-        DeclScope<Emitter> LocalScope(this, VD);
-        if (!visitGlobalInitializer(Init, *I))
-          return false;
+      if (std::optional<unsigned> Offset = this->allocateLocal(VD)) {
+        if (Init)
+          return this->visitLocalInitializer(Init, *Offset);
       }
-
-      // Return a pointer to the global.
-      if (!this->emitGetPtrGlobal(*I, VD))
-        return false;
-      return this->emitRetValue(VD);
     }
+    return true;
   }
 
-  return this->bail(VD);
+  return false;
 }
 
 template <class Emitter>

diff --git a/clang/lib/AST/Interp/ByteCodeExprGen.h b/clang/lib/AST/Interp/ByteCodeExprGen.h
index a873631..89782bd 100644 (file)
--- a/clang/lib/AST/Interp/ByteCodeExprGen.h
+++ b/clang/lib/AST/Interp/ByteCodeExprGen.h
@@ -132,6 +132,8 @@ protected:
   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);
@@ -234,6 +236,12 @@ private:
     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;
@@ -260,6 +268,11 @@ extern template class ByteCodeExprGen<EvalEmitter>;
 /// Scope chain managing the variable lifetimes.
 template <class Emitter> class VariableScope {
 public:
+  VariableScope(ByteCodeExprGen<Emitter> *Ctx)
+      : Ctx(Ctx), Parent(Ctx->VarScope) {
+    Ctx->VarScope = this;
+  }
+
   virtual ~VariableScope() { Ctx->VarScope = this->Parent; }
 
   void add(const Scope::Local &Local, bool IsExtended) {
@@ -284,11 +297,6 @@ public:
   VariableScope *getParent() { return Parent; }
 
 protected:
-  VariableScope(ByteCodeExprGen<Emitter> *Ctx)
-      : Ctx(Ctx), Parent(Ctx->VarScope) {
-    Ctx->VarScope = this;
-  }
-
   /// ByteCodeExprGen instance.
   ByteCodeExprGen<Emitter> *Ctx;
   /// Link to the parent scope.

diff --git a/clang/lib/AST/Interp/ByteCodeStmtGen.cpp b/clang/lib/AST/Interp/ByteCodeStmtGen.cpp
index 60506a7..af97c57 100644 (file)
--- a/clang/lib/AST/Interp/ByteCodeStmtGen.cpp
+++ b/clang/lib/AST/Interp/ByteCodeStmtGen.cpp
@@ -207,7 +207,7 @@ 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;
     }
@@ -391,41 +391,6 @@ bool ByteCodeStmtGen<Emitter>::visitContinueStmt(const ContinueStmt *S) {
   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 {
 

diff --git a/clang/lib/AST/Interp/ByteCodeStmtGen.h b/clang/lib/AST/Interp/ByteCodeStmtGen.h
index 2385c3e..829e199 100644 (file)
--- a/clang/lib/AST/Interp/ByteCodeStmtGen.h
+++ b/clang/lib/AST/Interp/ByteCodeStmtGen.h
@@ -63,10 +63,6 @@ private:
   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;
 

diff --git a/clang/lib/AST/Interp/Program.cpp b/clang/lib/AST/Interp/Program.cpp
index 067f6d9..5305ddd 100644 (file)
--- a/clang/lib/AST/Interp/Program.cpp
+++ b/clang/lib/AST/Interp/Program.cpp
@@ -125,11 +125,12 @@ std::optional<unsigned> Program::getGlobal(const ValueDecl *VD) {
   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;
   }
@@ -157,6 +158,7 @@ std::optional<unsigned> Program::getOrCreateDummy(const ParmVarDecl *PD) {
 
 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();

diff --git a/clang/lib/AST/Interp/Program.h b/clang/lib/AST/Interp/Program.h
index f49ca6d..5a80dd1 100644 (file)
--- a/clang/lib/AST/Interp/Program.h
+++ b/clang/lib/AST/Interp/Program.h
@@ -79,7 +79,8 @@ public:
   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);