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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
