[clang] [clang][Interp] Only evaluate the source array initialization of an `ArrayInitLoopExpr` once (PR #68039)
via cfe-commits
cfe-commits at lists.llvm.org
Mon Oct 2 14:11:47 PDT 2023
https://github.com/isuckatcs created https://github.com/llvm/llvm-project/pull/68039
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.
>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/2] 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/2] 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
More information about the cfe-commits
mailing list