[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:53:46 PDT 2023
https://github.com/isuckatcs updated https://github.com/llvm/llvm-project/pull/68039
>From 6748bd2171cca24c46ade12d57b57e97c3312501 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 e9e20b222d5d34f..7d38d82f25331e4 100644
--- a/clang/lib/AST/Interp/ByteCodeExprGen.cpp
+++ b/clang/lib/AST/Interp/ByteCodeExprGen.cpp
@@ -810,9 +810,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());
@@ -845,8 +856,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 2049dab140eaaae..a6c546384580b9a 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>
@@ -220,6 +221,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(PrimType T, QualType QT, const Expr *E);
@@ -304,6 +306,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>;
@@ -479,6 +485,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 98ac7bdc5b07fcb23b8852b39179ecb711431f7c 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 7d38d82f25331e4..7e4bf0f036a11db 100644
--- a/clang/lib/AST/Interp/ByteCodeExprGen.cpp
+++ b/clang/lib/AST/Interp/ByteCodeExprGen.cpp
@@ -811,18 +811,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();
@@ -856,8 +846,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 a6c546384580b9a..6a25457f946ad0f 100644
--- a/clang/lib/AST/Interp/ByteCodeExprGen.h
+++ b/clang/lib/AST/Interp/ByteCodeExprGen.h
@@ -306,10 +306,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>;
@@ -487,21 +483,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 530cf3a695db2d12e45092897fa82d11524a5cc6 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 7e4bf0f036a11db..6569e63a551cf0a 100644
--- a/clang/lib/AST/Interp/ByteCodeExprGen.cpp
+++ b/clang/lib/AST/Interp/ByteCodeExprGen.cpp
@@ -846,12 +846,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 6a25457f946ad0f..6042fe6c0acdf87 100644
--- a/clang/lib/AST/Interp/ByteCodeExprGen.h
+++ b/clang/lib/AST/Interp/ByteCodeExprGen.h
@@ -485,19 +485,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});
@@ -507,13 +509,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 4a1acf32dc32c501d0f06ce52f7a012d0ae2bb15 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 6569e63a551cf0a..344dd27933a794e 100644
--- a/clang/lib/AST/Interp/ByteCodeExprGen.cpp
+++ b/clang/lib/AST/Interp/ByteCodeExprGen.cpp
@@ -812,7 +812,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 a2cac4ce48aaf865278e982edf47f7b79a66532d 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 344dd27933a794e..90d59b0f4e0ee0a 100644
--- a/clang/lib/AST/Interp/ByteCodeExprGen.cpp
+++ b/clang/lib/AST/Interp/ByteCodeExprGen.cpp
@@ -811,10 +811,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());
@@ -847,12 +843,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 6042fe6c0acdf87..2049dab140eaaae 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>
@@ -221,7 +220,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(PrimType T, QualType QT, const Expr *E);
@@ -481,43 +479,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