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

[via cfe-commits]

llvmbot wrote:


<!--LLVM PR SUMMARY COMMENT-->

@llvm/pr-subscribers-clang

<details>
<summary>Changes</summary>

At the moment in `Interp` the source array initialization (`getCommonExpr()`) of an `ArrayInitLoopExpr` is evaluated during every iteration, when it should only be evaluated once.

The initializer is always wrapped inside an `OpaqueValueExpr`, which in `ExprConstant` is evaluated once per scope and their result is stored so that the next time `ExprConstant` sees the same expression, it can return the result only.

This patch intents to achieve a similar functionality inside `Interp` by storing the result of the `OpaqueValueExpr` in a local variable.

---
Full diff: https://github.com/llvm/llvm-project/pull/68039.diff


3 Files Affected:

- (modified) clang/lib/AST/Interp/ByteCodeExprGen.cpp (+7-3) 
- (modified) clang/lib/AST/Interp/ByteCodeExprGen.h (+39) 
- (modified) clang/test/AST/Interp/arrays.cpp (+1-5) 


``````````diff
diff --git a/clang/lib/AST/Interp/ByteCodeExprGen.cpp b/clang/lib/AST/Interp/ByteCodeExprGen.cpp
index 46906377863bd74..d79cc77c5c38952 100644
--- a/clang/lib/AST/Interp/ByteCodeExprGen.cpp
+++ b/clang/lib/AST/Interp/ByteCodeExprGen.cpp
@@ -792,9 +792,10 @@ 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.
+
+  StoredOpaqueValueScope<Emitter> StoredOpaqueScope(this);
+  StoredOpaqueScope.VisitAndStoreOpaqueValue(E->getCommonExpr());
+
   const Expr *SubExpr = E->getSubExpr();
   size_t Size = E->getArraySize().getZExtValue();
   std::optional<PrimType> ElemT = classify(SubExpr->getType());
@@ -827,6 +828,9 @@ 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());
diff --git a/clang/lib/AST/Interp/ByteCodeExprGen.h b/clang/lib/AST/Interp/ByteCodeExprGen.h
index 47a3f75f13459d0..1a66a8b76dcbf41 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);
@@ -478,6 +480,43 @@ 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 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 {

``````````

</details>


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