[clang] [clang][Interp] Only evaluate the source array initialization of an `ArrayInitLoopExpr` once (PR #68039)

via cfe-commits cfe-commits at lists.llvm.org
Sun Oct 15 09:35:45 PDT 2023


https://github.com/isuckatcs updated https://github.com/llvm/llvm-project/pull/68039

>From baf0fc082f2cfa86346a93b22c39b92e9e7e261b Mon Sep 17 00:00:00 2001
From: isuckatcs <65320245+isuckatcs at users.noreply.github.com>
Date: Mon, 2 Oct 2023 22:29:14 +0200
Subject: [PATCH 1/5] impl

---
 clang/lib/AST/Interp/ByteCodeExprGen.cpp | 21 +++++++++++++++++---
 clang/lib/AST/Interp/ByteCodeExprGen.h   | 25 ++++++++++++++++++++++++
 clang/test/AST/Interp/arrays.cpp         |  6 +-----
 3 files changed, 44 insertions(+), 8 deletions(-)

diff --git a/clang/lib/AST/Interp/ByteCodeExprGen.cpp b/clang/lib/AST/Interp/ByteCodeExprGen.cpp
index 46906377863bd74..b2cf34ac8c45bbb 100644
--- a/clang/lib/AST/Interp/ByteCodeExprGen.cpp
+++ b/clang/lib/AST/Interp/ByteCodeExprGen.cpp
@@ -792,9 +792,20 @@ bool ByteCodeExprGen<Emitter>::VisitArrayInitLoopExpr(
     const ArrayInitLoopExpr *E) {
   assert(Initializing);
   assert(!DiscardResult);
-  // TODO: This compiles to quite a lot of bytecode if the array is larger.
-  //   Investigate compiling this to a loop, or at least try to use
-  //   the AILE's Common expr.
+
+  const auto *CommonExpr = E->getCommonExpr();
+  std::optional<PrimType> CommonTy = classify(CommonExpr);
+
+  std::optional<unsigned> LocalIndex = this->allocateLocalPrimitive(CommonExpr, *CommonTy, CommonExpr->getType().isConstQualified());
+  if (!LocalIndex)
+    return false;
+  if (!this->visit(CommonExpr))
+    return false;
+  if(!this->emitSetLocal(*CommonTy, *LocalIndex, E))
+    return false;
+    
+  StoredOpaqueValueScope<Emitter> KnownOpaqueScope(this, *LocalIndex);
+
   const Expr *SubExpr = E->getSubExpr();
   size_t Size = E->getArraySize().getZExtValue();
   std::optional<PrimType> ElemT = classify(SubExpr->getType());
@@ -827,8 +838,12 @@ bool ByteCodeExprGen<Emitter>::VisitArrayInitLoopExpr(
 
 template <class Emitter>
 bool ByteCodeExprGen<Emitter>::VisitOpaqueValueExpr(const OpaqueValueExpr *E) {
+  if(IgnoreOpaqueValue)
+    return this->emitGetLocal(*classify(E), *OpaqueValueIndex, E);
+
   if (Initializing)
     return this->visitInitializer(E->getSourceExpr());
+  
   return this->visit(E->getSourceExpr());
 }
 
diff --git a/clang/lib/AST/Interp/ByteCodeExprGen.h b/clang/lib/AST/Interp/ByteCodeExprGen.h
index 47a3f75f13459d0..f58def7a3245ca6 100644
--- a/clang/lib/AST/Interp/ByteCodeExprGen.h
+++ b/clang/lib/AST/Interp/ByteCodeExprGen.h
@@ -36,6 +36,7 @@ template <class Emitter> class DeclScope;
 template <class Emitter> class OptionScope;
 template <class Emitter> class ArrayIndexScope;
 template <class Emitter> class SourceLocScope;
+template <class Emitter> class StoredOpaqueValueScope;
 
 /// Compilation context for expressions.
 template <class Emitter>
@@ -219,6 +220,7 @@ class ByteCodeExprGen : public ConstStmtVisitor<ByteCodeExprGen<Emitter>, bool>,
   friend class OptionScope<Emitter>;
   friend class ArrayIndexScope<Emitter>;
   friend class SourceLocScope<Emitter>;
+  friend class StoredOpaqueValueScope<Emitter>;
 
   /// Emits a zero initializer.
   bool visitZeroInitializer(QualType QT, const Expr *E);
@@ -303,6 +305,10 @@ class ByteCodeExprGen : public ConstStmtVisitor<ByteCodeExprGen<Emitter>, bool>,
   /// Flag inidicating if we're initializing an already created
   /// variable. This is set in visitInitializer().
   bool Initializing = false;
+
+  /// Flag indicating if we ignore an OpaqueValueExpr.
+  bool IgnoreOpaqueValue = false;
+  std::optional<uint64_t> OpaqueValueIndex;
 };
 
 extern template class ByteCodeExprGen<ByteCodeEmitter>;
@@ -478,6 +484,25 @@ template <class Emitter> class SourceLocScope final {
   bool Enabled = false;
 };
 
+template <class Emitter> class StoredOpaqueValueScope final {
+public:
+  StoredOpaqueValueScope(ByteCodeExprGen<Emitter> *Ctx, uint64_t LocalIndex, bool ignore = true)
+      : Ctx(Ctx), OldIgnoreValue(Ctx->IgnoreOpaqueValue), OldLocalIndex(Ctx->OpaqueValueIndex) {
+    Ctx->IgnoreOpaqueValue = ignore;
+    Ctx->OpaqueValueIndex = LocalIndex;
+  }
+
+  ~StoredOpaqueValueScope() {
+      Ctx->IgnoreOpaqueValue = OldIgnoreValue;
+      Ctx->OpaqueValueIndex = OldLocalIndex;
+  }
+
+private:
+  ByteCodeExprGen<Emitter> *Ctx;
+  bool OldIgnoreValue;
+  std::optional<uint64_t> OldLocalIndex;
+};
+
 } // namespace interp
 } // namespace clang
 
diff --git a/clang/test/AST/Interp/arrays.cpp b/clang/test/AST/Interp/arrays.cpp
index 281835f828bbd7c..1f8908f2bed0b24 100644
--- a/clang/test/AST/Interp/arrays.cpp
+++ b/clang/test/AST/Interp/arrays.cpp
@@ -352,9 +352,6 @@ namespace ZeroInit {
 }
 
 namespace ArrayInitLoop {
-  /// FIXME: The ArrayInitLoop for the decomposition initializer in g() has
-  /// f(n) as its CommonExpr. We need to evaluate that exactly once and not
-  /// N times as we do right now.
   struct X {
       int arr[3];
   };
@@ -366,8 +363,7 @@ namespace ArrayInitLoop {
       auto [a, b, c] = f(n).arr;
       return a + b + c;
   }
-  static_assert(g() == 6); // expected-error {{failed}} \
-                           // expected-note {{15 == 6}}
+  static_assert(g() == 6);
 }
 
 namespace StringZeroFill {

>From 3724f695e7699840998d8af16a60f02f90f64b95 Mon Sep 17 00:00:00 2001
From: isuckatcs <65320245+isuckatcs at users.noreply.github.com>
Date: Mon, 2 Oct 2023 22:51:58 +0200
Subject: [PATCH 2/5] cleanup

---
 clang/lib/AST/Interp/ByteCodeExprGen.cpp | 18 +++---------
 clang/lib/AST/Interp/ByteCodeExprGen.h   | 36 ++++++++++++++++--------
 2 files changed, 28 insertions(+), 26 deletions(-)

diff --git a/clang/lib/AST/Interp/ByteCodeExprGen.cpp b/clang/lib/AST/Interp/ByteCodeExprGen.cpp
index b2cf34ac8c45bbb..27848e7bb732691 100644
--- a/clang/lib/AST/Interp/ByteCodeExprGen.cpp
+++ b/clang/lib/AST/Interp/ByteCodeExprGen.cpp
@@ -793,18 +793,8 @@ bool ByteCodeExprGen<Emitter>::VisitArrayInitLoopExpr(
   assert(Initializing);
   assert(!DiscardResult);
 
-  const auto *CommonExpr = E->getCommonExpr();
-  std::optional<PrimType> CommonTy = classify(CommonExpr);
-
-  std::optional<unsigned> LocalIndex = this->allocateLocalPrimitive(CommonExpr, *CommonTy, CommonExpr->getType().isConstQualified());
-  if (!LocalIndex)
-    return false;
-  if (!this->visit(CommonExpr))
-    return false;
-  if(!this->emitSetLocal(*CommonTy, *LocalIndex, E))
-    return false;
-    
-  StoredOpaqueValueScope<Emitter> KnownOpaqueScope(this, *LocalIndex);
+  StoredOpaqueValueScope<Emitter> StoredOpaqueScope(this);
+  StoredOpaqueScope.VisitAndStoreOpaqueValue(E->getCommonExpr());
 
   const Expr *SubExpr = E->getSubExpr();
   size_t Size = E->getArraySize().getZExtValue();
@@ -838,8 +828,8 @@ bool ByteCodeExprGen<Emitter>::VisitArrayInitLoopExpr(
 
 template <class Emitter>
 bool ByteCodeExprGen<Emitter>::VisitOpaqueValueExpr(const OpaqueValueExpr *E) {
-  if(IgnoreOpaqueValue)
-    return this->emitGetLocal(*classify(E), *OpaqueValueIndex, E);
+  if(OpaqueExprs.contains(E))
+    return this->emitGetLocal(*classify(E), OpaqueExprs[E], E);
 
   if (Initializing)
     return this->visitInitializer(E->getSourceExpr());
diff --git a/clang/lib/AST/Interp/ByteCodeExprGen.h b/clang/lib/AST/Interp/ByteCodeExprGen.h
index f58def7a3245ca6..0e69eee128f45a7 100644
--- a/clang/lib/AST/Interp/ByteCodeExprGen.h
+++ b/clang/lib/AST/Interp/ByteCodeExprGen.h
@@ -305,10 +305,6 @@ class ByteCodeExprGen : public ConstStmtVisitor<ByteCodeExprGen<Emitter>, bool>,
   /// Flag inidicating if we're initializing an already created
   /// variable. This is set in visitInitializer().
   bool Initializing = false;
-
-  /// Flag indicating if we ignore an OpaqueValueExpr.
-  bool IgnoreOpaqueValue = false;
-  std::optional<uint64_t> OpaqueValueIndex;
 };
 
 extern template class ByteCodeExprGen<ByteCodeEmitter>;
@@ -486,21 +482,37 @@ template <class Emitter> class SourceLocScope final {
 
 template <class Emitter> class StoredOpaqueValueScope final {
 public:
-  StoredOpaqueValueScope(ByteCodeExprGen<Emitter> *Ctx, uint64_t LocalIndex, bool ignore = true)
-      : Ctx(Ctx), OldIgnoreValue(Ctx->IgnoreOpaqueValue), OldLocalIndex(Ctx->OpaqueValueIndex) {
-    Ctx->IgnoreOpaqueValue = ignore;
-    Ctx->OpaqueValueIndex = LocalIndex;
+  StoredOpaqueValueScope(ByteCodeExprGen<Emitter> *Ctx) : Ctx(Ctx) {}
+
+  bool VisitAndStoreOpaqueValue(const OpaqueValueExpr* Ove) {
+    assert(Ove && "OpaqueValueExpr is a nullptr!");
+    assert(!Ctx->OpaqueExprs.contains(Ove) && "OpaqueValueExpr already stored!");
+
+    std::optional<PrimType> CommonTy = Ctx->classify(Ove);
+    std::optional<unsigned> LocalIndex = Ctx->allocateLocalPrimitive(Ove, *CommonTy, Ove->getType().isConstQualified());
+    if (!LocalIndex)
+      return false;
+
+    if (!Ctx->visit(Ove))
+      return false;
+
+    if(!Ctx->emitSetLocal(*CommonTy, *LocalIndex, Ove))
+      return false;
+
+    Ctx->OpaqueExprs.insert({Ove, *LocalIndex});
+    StoredValues.emplace_back(Ove);
+
+    return true;
   }
 
   ~StoredOpaqueValueScope() {
-      Ctx->IgnoreOpaqueValue = OldIgnoreValue;
-      Ctx->OpaqueValueIndex = OldLocalIndex;
+    for(const auto *SV : StoredValues)
+      Ctx->OpaqueExprs.erase(SV);
   }
 
 private:
   ByteCodeExprGen<Emitter> *Ctx;
-  bool OldIgnoreValue;
-  std::optional<uint64_t> OldLocalIndex;
+  std::vector<const OpaqueValueExpr*> StoredValues;
 };
 
 } // namespace interp

>From ed93b55bb18e3d7c1ccbcec2f7f6f63b2f2a8bbf Mon Sep 17 00:00:00 2001
From: isuckatcs <65320245+isuckatcs at users.noreply.github.com>
Date: Mon, 2 Oct 2023 23:12:02 +0200
Subject: [PATCH 3/5] applied clang-format

---
 clang/lib/AST/Interp/ByteCodeExprGen.cpp |  3 +--
 clang/lib/AST/Interp/ByteCodeExprGen.h   | 14 ++++++++------
 2 files changed, 9 insertions(+), 8 deletions(-)

diff --git a/clang/lib/AST/Interp/ByteCodeExprGen.cpp b/clang/lib/AST/Interp/ByteCodeExprGen.cpp
index 27848e7bb732691..d79cc77c5c38952 100644
--- a/clang/lib/AST/Interp/ByteCodeExprGen.cpp
+++ b/clang/lib/AST/Interp/ByteCodeExprGen.cpp
@@ -828,12 +828,11 @@ bool ByteCodeExprGen<Emitter>::VisitArrayInitLoopExpr(
 
 template <class Emitter>
 bool ByteCodeExprGen<Emitter>::VisitOpaqueValueExpr(const OpaqueValueExpr *E) {
-  if(OpaqueExprs.contains(E))
+  if (OpaqueExprs.contains(E))
     return this->emitGetLocal(*classify(E), OpaqueExprs[E], E);
 
   if (Initializing)
     return this->visitInitializer(E->getSourceExpr());
-  
   return this->visit(E->getSourceExpr());
 }
 
diff --git a/clang/lib/AST/Interp/ByteCodeExprGen.h b/clang/lib/AST/Interp/ByteCodeExprGen.h
index 0e69eee128f45a7..1a66a8b76dcbf41 100644
--- a/clang/lib/AST/Interp/ByteCodeExprGen.h
+++ b/clang/lib/AST/Interp/ByteCodeExprGen.h
@@ -484,19 +484,21 @@ template <class Emitter> class StoredOpaqueValueScope final {
 public:
   StoredOpaqueValueScope(ByteCodeExprGen<Emitter> *Ctx) : Ctx(Ctx) {}
 
-  bool VisitAndStoreOpaqueValue(const OpaqueValueExpr* Ove) {
+  bool VisitAndStoreOpaqueValue(const OpaqueValueExpr *Ove) {
     assert(Ove && "OpaqueValueExpr is a nullptr!");
-    assert(!Ctx->OpaqueExprs.contains(Ove) && "OpaqueValueExpr already stored!");
+    assert(!Ctx->OpaqueExprs.contains(Ove) &&
+           "OpaqueValueExpr already stored!");
 
     std::optional<PrimType> CommonTy = Ctx->classify(Ove);
-    std::optional<unsigned> LocalIndex = Ctx->allocateLocalPrimitive(Ove, *CommonTy, Ove->getType().isConstQualified());
+    std::optional<unsigned> LocalIndex = Ctx->allocateLocalPrimitive(
+        Ove, *CommonTy, Ove->getType().isConstQualified());
     if (!LocalIndex)
       return false;
 
     if (!Ctx->visit(Ove))
       return false;
 
-    if(!Ctx->emitSetLocal(*CommonTy, *LocalIndex, Ove))
+    if (!Ctx->emitSetLocal(*CommonTy, *LocalIndex, Ove))
       return false;
 
     Ctx->OpaqueExprs.insert({Ove, *LocalIndex});
@@ -506,13 +508,13 @@ template <class Emitter> class StoredOpaqueValueScope final {
   }
 
   ~StoredOpaqueValueScope() {
-    for(const auto *SV : StoredValues)
+    for (const auto *SV : StoredValues)
       Ctx->OpaqueExprs.erase(SV);
   }
 
 private:
   ByteCodeExprGen<Emitter> *Ctx;
-  std::vector<const OpaqueValueExpr*> StoredValues;
+  std::vector<const OpaqueValueExpr *> StoredValues;
 };
 
 } // namespace interp

>From 0998a51f9a3006cbf4a888a6d8f8ba0adc6699d7 Mon Sep 17 00:00:00 2001
From: isuckatcs <65320245+isuckatcs at users.noreply.github.com>
Date: Mon, 2 Oct 2023 23:22:36 +0200
Subject: [PATCH 4/5] return if ove evaluation fails

---
 clang/lib/AST/Interp/ByteCodeExprGen.cpp | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/clang/lib/AST/Interp/ByteCodeExprGen.cpp b/clang/lib/AST/Interp/ByteCodeExprGen.cpp
index d79cc77c5c38952..0b61311158066da 100644
--- a/clang/lib/AST/Interp/ByteCodeExprGen.cpp
+++ b/clang/lib/AST/Interp/ByteCodeExprGen.cpp
@@ -794,7 +794,8 @@ bool ByteCodeExprGen<Emitter>::VisitArrayInitLoopExpr(
   assert(!DiscardResult);
 
   StoredOpaqueValueScope<Emitter> StoredOpaqueScope(this);
-  StoredOpaqueScope.VisitAndStoreOpaqueValue(E->getCommonExpr());
+  if (!StoredOpaqueScope.VisitAndStoreOpaqueValue(E->getCommonExpr()))
+    return false;
 
   const Expr *SubExpr = E->getSubExpr();
   size_t Size = E->getArraySize().getZExtValue();

>From 9e3c632c88a24751231ab06ce5aea0ac9b77be04 Mon Sep 17 00:00:00 2001
From: isuckatcs <65320245+isuckatcs at users.noreply.github.com>
Date: Sun, 15 Oct 2023 18:35:27 +0200
Subject: [PATCH 5/5] addressed comments

---
 clang/lib/AST/Interp/ByteCodeExprGen.cpp | 32 ++++++++++++++-----
 clang/lib/AST/Interp/ByteCodeExprGen.h   | 39 ------------------------
 clang/test/AST/Interp/arrays.cpp         |  2 +-
 3 files changed, 25 insertions(+), 48 deletions(-)

diff --git a/clang/lib/AST/Interp/ByteCodeExprGen.cpp b/clang/lib/AST/Interp/ByteCodeExprGen.cpp
index 0b61311158066da..7a91d6783a206ff 100644
--- a/clang/lib/AST/Interp/ByteCodeExprGen.cpp
+++ b/clang/lib/AST/Interp/ByteCodeExprGen.cpp
@@ -793,10 +793,6 @@ bool ByteCodeExprGen<Emitter>::VisitArrayInitLoopExpr(
   assert(Initializing);
   assert(!DiscardResult);
 
-  StoredOpaqueValueScope<Emitter> StoredOpaqueScope(this);
-  if (!StoredOpaqueScope.VisitAndStoreOpaqueValue(E->getCommonExpr()))
-    return false;
-
   const Expr *SubExpr = E->getSubExpr();
   size_t Size = E->getArraySize().getZExtValue();
   std::optional<PrimType> ElemT = classify(SubExpr->getType());
@@ -829,12 +825,32 @@ bool ByteCodeExprGen<Emitter>::VisitArrayInitLoopExpr(
 
 template <class Emitter>
 bool ByteCodeExprGen<Emitter>::VisitOpaqueValueExpr(const OpaqueValueExpr *E) {
-  if (OpaqueExprs.contains(E))
-    return this->emitGetLocal(*classify(E), OpaqueExprs[E], E);
-
   if (Initializing)
     return this->visitInitializer(E->getSourceExpr());
-  return this->visit(E->getSourceExpr());
+
+  PrimType CacheVariableTy = classify(E).value_or(PT_Ptr);
+  if (OpaqueExprs.contains(E))
+    return this->emitGetLocal(CacheVariableTy, OpaqueExprs[E], E);
+
+  if(!this->visit(E->getSourceExpr()))
+    return false;
+
+  // At this point we either have the evaluated source expression or a pointer to an object on the stack.
+  // We want to create a local variable that stores this value.
+  std::optional<unsigned> LocalIndex = allocateLocalPrimitive(E, CacheVariableTy, true);
+  if (!LocalIndex)
+    return false;
+  if (!this->emitSetLocal(CacheVariableTy, *LocalIndex, E))
+    return false;
+  
+  // Here the local variable is created but the value is removed from the stack, so we put it back, because the caller might need it.
+  if(!this->emitGetLocal(CacheVariableTy, *LocalIndex, E))
+    return false;
+
+  // FIXME: Ideally the cached value should be cleaned up later.
+  OpaqueExprs.insert({E, *LocalIndex});
+
+  return true;
 }
 
 template <class Emitter>
diff --git a/clang/lib/AST/Interp/ByteCodeExprGen.h b/clang/lib/AST/Interp/ByteCodeExprGen.h
index 1a66a8b76dcbf41..47a3f75f13459d0 100644
--- a/clang/lib/AST/Interp/ByteCodeExprGen.h
+++ b/clang/lib/AST/Interp/ByteCodeExprGen.h
@@ -36,7 +36,6 @@ template <class Emitter> class DeclScope;
 template <class Emitter> class OptionScope;
 template <class Emitter> class ArrayIndexScope;
 template <class Emitter> class SourceLocScope;
-template <class Emitter> class StoredOpaqueValueScope;
 
 /// Compilation context for expressions.
 template <class Emitter>
@@ -220,7 +219,6 @@ class ByteCodeExprGen : public ConstStmtVisitor<ByteCodeExprGen<Emitter>, bool>,
   friend class OptionScope<Emitter>;
   friend class ArrayIndexScope<Emitter>;
   friend class SourceLocScope<Emitter>;
-  friend class StoredOpaqueValueScope<Emitter>;
 
   /// Emits a zero initializer.
   bool visitZeroInitializer(QualType QT, const Expr *E);
@@ -480,43 +478,6 @@ template <class Emitter> class SourceLocScope final {
   bool Enabled = false;
 };
 
-template <class Emitter> class StoredOpaqueValueScope final {
-public:
-  StoredOpaqueValueScope(ByteCodeExprGen<Emitter> *Ctx) : Ctx(Ctx) {}
-
-  bool VisitAndStoreOpaqueValue(const OpaqueValueExpr *Ove) {
-    assert(Ove && "OpaqueValueExpr is a nullptr!");
-    assert(!Ctx->OpaqueExprs.contains(Ove) &&
-           "OpaqueValueExpr already stored!");
-
-    std::optional<PrimType> CommonTy = Ctx->classify(Ove);
-    std::optional<unsigned> LocalIndex = Ctx->allocateLocalPrimitive(
-        Ove, *CommonTy, Ove->getType().isConstQualified());
-    if (!LocalIndex)
-      return false;
-
-    if (!Ctx->visit(Ove))
-      return false;
-
-    if (!Ctx->emitSetLocal(*CommonTy, *LocalIndex, Ove))
-      return false;
-
-    Ctx->OpaqueExprs.insert({Ove, *LocalIndex});
-    StoredValues.emplace_back(Ove);
-
-    return true;
-  }
-
-  ~StoredOpaqueValueScope() {
-    for (const auto *SV : StoredValues)
-      Ctx->OpaqueExprs.erase(SV);
-  }
-
-private:
-  ByteCodeExprGen<Emitter> *Ctx;
-  std::vector<const OpaqueValueExpr *> StoredValues;
-};
-
 } // namespace interp
 } // namespace clang
 
diff --git a/clang/test/AST/Interp/arrays.cpp b/clang/test/AST/Interp/arrays.cpp
index 1f8908f2bed0b24..fcbaa31e8608ec3 100644
--- a/clang/test/AST/Interp/arrays.cpp
+++ b/clang/test/AST/Interp/arrays.cpp
@@ -363,7 +363,7 @@ namespace ArrayInitLoop {
       auto [a, b, c] = f(n).arr;
       return a + b + c;
   }
-  static_assert(g() == 6);
+  static_assert(g() == 6, "");
 }
 
 namespace StringZeroFill {



More information about the cfe-commits mailing list