[clang][Interp] Cleaning up FIXMEs added during ArrayInitLoopExpr implementation.

[isuckatcs]

This patch removes the two FIXMEs that were added with patches related to the expression mentioned in the title.

[llvmbot]

@llvm/pr-subscribers-clang

Author: None (isuckatcs)

Changes

This patch removes the two FIXMEs that were added with patches related to the expression mentioned in the title.

---

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

2 Files Affected:

• (modified) clang/lib/AST/Interp/ByteCodeExprGen.cpp (+8-12)
• (modified) clang/lib/AST/Interp/ByteCodeExprGen.h (+20)

diff --git a/clang/lib/AST/Interp/ByteCodeExprGen.cpp b/clang/lib/AST/Interp/ByteCodeExprGen.cpp
index 1b33c69b93aa4b9..8d18698df60c2c1 100644
--- a/clang/lib/AST/Interp/ByteCodeExprGen.cpp
+++ b/clang/lib/AST/Interp/ByteCodeExprGen.cpp
@@ -816,22 +816,18 @@ bool ByteCodeExprGen<Emitter>::VisitArrayInitLoopExpr(
     const ArrayInitLoopExpr *E) {
   assert(Initializing);
   assert(!DiscardResult);
+
+  // We visit the common opaque expression here once so we have its value
+  // cached.
+  if (!this->discard(E->getCommonExpr()))
+    return false;
+
   // TODO: This compiles to quite a lot of bytecode if the array is larger.
   //   Investigate compiling this to a loop.
-
   const Expr *SubExpr = E->getSubExpr();
-  const Expr *CommonExpr = E->getCommonExpr();
   size_t Size = E->getArraySize().getZExtValue();
   std::optional<PrimType> ElemT = classify(SubExpr->getType());
 
-  // If the common expression is an opaque expression, we visit it
-  // here once so we have its value cached.
-  // FIXME: This might be necessary (or useful) for all expressions.
-  if (isa<OpaqueValueExpr>(CommonExpr)) {
-    if (!this->discard(CommonExpr))
-      return false;
-  }
-
   // So, every iteration, we execute an assignment here
   // where the LHS is on the stack (the target array)
   // and the RHS is our SubExpr.
@@ -882,13 +878,13 @@ bool ByteCodeExprGen<Emitter>::VisitOpaqueValueExpr(const OpaqueValueExpr *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.
+  // so we put it back if the caller needs it.
   if (!DiscardResult) {
     if (!this->emitGetLocal(SubExprT, *LocalIndex, E))
       return false;
   }
 
-  // FIXME: Ideally the cached value should be cleaned up later.
+  // This is cleaned up when the local variable is destroyed.
   OpaqueExprs.insert({E, *LocalIndex});
 
   return true;
diff --git a/clang/lib/AST/Interp/ByteCodeExprGen.h b/clang/lib/AST/Interp/ByteCodeExprGen.h
index 83986d3dd579ed6..774d0b25f4ad56d 100644
--- a/clang/lib/AST/Interp/ByteCodeExprGen.h
+++ b/clang/lib/AST/Interp/ByteCodeExprGen.h
@@ -360,6 +360,7 @@ template <class Emitter> class LocalScope : public VariableScope<Emitter> {
     if (!Idx)
       return;
     this->Ctx->emitDestroy(*Idx, SourceInfo{});
+    removeStoredOpaqueValues();
   }
 
   /// Overriden to support explicit destruction.
@@ -368,6 +369,7 @@ template <class Emitter> class LocalScope : public VariableScope<Emitter> {
       return;
     this->emitDestructors();
     this->Ctx->emitDestroy(*Idx, SourceInfo{});
+    removeStoredOpaqueValues();
     this->Idx = std::nullopt;
   }
 
@@ -389,10 +391,28 @@ template <class Emitter> class LocalScope : public VariableScope<Emitter> {
       if (!Local.Desc->isPrimitive() && !Local.Desc->isPrimitiveArray()) {
         this->Ctx->emitGetPtrLocal(Local.Offset, SourceInfo{});
         this->Ctx->emitRecordDestruction(Local.Desc);
+        removeIfStoredOpaqueValue(Local);
       }
     }
   }
 
+  void removeStoredOpaqueValues() {
+    if (!Idx)
+      return;
+
+    for (Scope::Local &Local : this->Ctx->Descriptors[*Idx]) {
+      removeIfStoredOpaqueValue(Local);
+    }
+  }
+
+  void removeIfStoredOpaqueValue(const Scope::Local &Local) {
+    if (auto *OVE =
+            llvm::dyn_cast_or_null<OpaqueValueExpr>(Local.Desc->asExpr());
+        OVE && this->Ctx->OpaqueExprs.contains(OVE)) {
+      this->Ctx->OpaqueExprs.erase(OVE);
+    };
+  }
+
   /// Index of the scope in the chain.
   std::optional<unsigned> Idx;
 };

[tbaederr]

Why is just leaving them in the map such a big deal?

[isuckatcs]

There might be cases like loops or recursive functions, where we might want to evaluate the same OpaqueValueExpr multiple times, but we will fail if it is left in the map. I'm just trying to be careful here.

[cor3ntin]

@tbaederr ping :)

[isuckatcs]

Ping

[github-actions]

✅ With the latest revision this PR passed the C/C++ code formatter.