[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:36:06 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 2a75b190bf628e7622d257c55d1468f64897e514 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 | 35 ++++++++++++++++-----
clang/lib/AST/Interp/ByteCodeExprGen.h | 39 ------------------------
clang/test/AST/Interp/arrays.cpp | 2 +-
3 files changed, 28 insertions(+), 48 deletions(-)
diff --git a/clang/lib/AST/Interp/ByteCodeExprGen.cpp b/clang/lib/AST/Interp/ByteCodeExprGen.cpp
index 0b61311158066da..be1dbdee4eea126 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,35 @@ 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