[PATCH] D134361: [clang][Interp] Fix copy constructors of structs with array members
This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.
Closed by commit rG33b52836de6e: [clang][Interp] Fix using default copy
constructors (authored by tbaeder).
Changed prior to commit:
https://reviews.llvm.org/D134361?vs=463522=467710#toc
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D134361/new/
https://reviews.llvm.org/D134361
Files:
clang/lib/AST/Interp/ByteCodeExprGen.cpp
clang/lib/AST/Interp/ByteCodeExprGen.h
clang/lib/AST/Interp/ByteCodeStmtGen.cpp
clang/test/AST/Interp/records.cpp
Index: clang/test/AST/Interp/records.cpp
===
--- clang/test/AST/Interp/records.cpp
+++ clang/test/AST/Interp/records.cpp
@@ -32,6 +32,9 @@
static_assert(ints.b == 30, "");
static_assert(ints.c, "");
static_assert(ints.getTen() == 10, "");
+static_assert(ints.numbers[0] == 1, "");
+static_assert(ints.numbers[1] == 2, "");
+static_assert(ints.numbers[2] == 3, "");
constexpr const BoolPair = ints.bp;
static_assert(BP.first, "");
@@ -62,11 +65,17 @@
static_assert(ints4.a == (40 * 50), "");
static_assert(ints4.b == 0, "");
static_assert(ints4.c, "");
-
-
-// FIXME: Implement initialization by DeclRefExpr.
-//constexpr Ints ints4 = ints3; TODO
-
+static_assert(ints4.numbers[0] == 1, "");
+static_assert(ints4.numbers[1] == 2, "");
+static_assert(ints4.numbers[2] == 3, "");
+
+constexpr Ints ints5 = ints4;
+static_assert(ints5.a == (40 * 50), "");
+static_assert(ints5.b == 0, "");
+static_assert(ints5.c, "");
+static_assert(ints5.numbers[0] == 1, "");
+static_assert(ints5.numbers[1] == 2, "");
+static_assert(ints5.numbers[2] == 3, "");
struct Ints2 {
Index: clang/lib/AST/Interp/ByteCodeStmtGen.cpp
===
--- clang/lib/AST/Interp/ByteCodeStmtGen.cpp
+++ clang/lib/AST/Interp/ByteCodeStmtGen.cpp
@@ -105,7 +105,7 @@
const Record::Field *F = R->getField(Member);
if (Optional T = this->classify(InitExpr->getType())) {
-if (!this->emitDupPtr(InitExpr))
+if (!this->emitThis(InitExpr))
return false;
if (!this->visit(InitExpr))
@@ -116,7 +116,7 @@
} else {
// Non-primitive case. Get a pointer to the field-to-initialize
// on the stack and call visitInitialzer() for it.
-if (!this->emitDupPtr(InitExpr))
+if (!this->emitThis(InitExpr))
return false;
if (!this->emitGetPtrField(F->Offset, InitExpr))
Index: clang/lib/AST/Interp/ByteCodeExprGen.h
===
--- clang/lib/AST/Interp/ByteCodeExprGen.h
+++ clang/lib/AST/Interp/ByteCodeExprGen.h
@@ -34,6 +34,7 @@
template class VariableScope;
template class DeclScope;
template class OptionScope;
+template class ArrayIndexScope;
/// Compilation context for expressions.
template
@@ -85,6 +86,8 @@
bool VisitConstantExpr(const ConstantExpr *E);
bool VisitUnaryExprOrTypeTraitExpr(const UnaryExprOrTypeTraitExpr *E);
bool VisitMemberExpr(const MemberExpr *E);
+ bool VisitArrayInitIndexExpr(const ArrayInitIndexExpr *E);
+ bool VisitOpaqueValueExpr(const OpaqueValueExpr *E);
protected:
bool visitExpr(const Expr *E) override;
@@ -199,6 +202,7 @@
friend class RecordScope;
friend class DeclScope;
friend class OptionScope;
+ friend class ArrayIndexScope;
/// Emits a zero initializer.
bool visitZeroInitializer(PrimType T, const Expr *E);
@@ -260,7 +264,7 @@
/// Current scope.
VariableScope *VarScope = nullptr;
- /// Current argument index.
+ /// Current argument index. Needed to emit ArrayInitIndexExpr.
llvm::Optional ArrayIndex;
/// Flag indicating if return value is to be discarded.
@@ -362,6 +366,20 @@
}
};
+template class ArrayIndexScope final {
+public:
+ ArrayIndexScope(ByteCodeExprGen *Ctx, uint64_t Index) : Ctx(Ctx) {
+OldArrayIndex = Ctx->ArrayIndex;
+Ctx->ArrayIndex = Index;
+ }
+
+ ~ArrayIndexScope() { Ctx->ArrayIndex = OldArrayIndex; }
+
+private:
+ ByteCodeExprGen *Ctx;
+ Optional OldArrayIndex;
+};
+
} // namespace interp
} // namespace clang
Index: clang/lib/AST/Interp/ByteCodeExprGen.cpp
===
--- clang/lib/AST/Interp/ByteCodeExprGen.cpp
+++ clang/lib/AST/Interp/ByteCodeExprGen.cpp
@@ -326,6 +326,18 @@
return false;
}
+template
+bool ByteCodeExprGen::VisitArrayInitIndexExpr(
+const ArrayInitIndexExpr *E) {
+ assert(ArrayIndex);
+ return this->emitConstUint64(*ArrayIndex, E);
+}
+
+template
+bool ByteCodeExprGen::VisitOpaqueValueExpr(const OpaqueValueExpr *E) {
+ return this->visit(E->getSourceExpr());
+}
+
template bool ByteCodeExprGen::discard(const Expr *E) {
OptionScope Scope(this, /*NewDiscardResult=*/true);
return this->Visit(E);
@@ -628,6 +640,32 @@
return true;
}
[PATCH] D134361: [clang][Interp] Fix copy constructors of structs with array members
tbaeder added inline comments. Comment at: clang/lib/AST/Interp/ByteCodeExprGen.cpp:649 + +if (!ElemT) + return false; shafik wrote: > Curious what case requires this check? I don't think there's a real test case for this, we could as well change the initializer to `*classify(SubExpr->getType());`. It's just the pattern used everywhere else. We could also use `classifyPrim` instead. Comment at: clang/lib/AST/Interp/ByteCodeExprGen.h:382 + ByteCodeExprGen *Ctx; + Optional OldArrayIndex; +}; shafik wrote: > Why an `Optional`? Your not checking it and I don't see how it won't be set? The first time we see an `ArrayInitLoopExpr`, the `ByteCodeExprGen::ArrayIndex` will be `None`, so we need to use this here (note: just writing this right now, it's pretty late for me, I didn't test //just// using a `uint64_t`) CHANGES SINCE LAST ACTION https://reviews.llvm.org/D134361/new/ https://reviews.llvm.org/D134361 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D134361: [clang][Interp] Fix copy constructors of structs with array members
shafik added inline comments. Comment at: clang/lib/AST/Interp/ByteCodeExprGen.cpp:649 + +if (!ElemT) + return false; Curious what case requires this check? Comment at: clang/lib/AST/Interp/ByteCodeExprGen.h:382 + ByteCodeExprGen *Ctx; + Optional OldArrayIndex; +}; Why an `Optional`? Your not checking it and I don't see how it won't be set? CHANGES SINCE LAST ACTION https://reviews.llvm.org/D134361/new/ https://reviews.llvm.org/D134361 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D134361: [clang][Interp] Fix copy constructors of structs with array members
aaron.ballman accepted this revision. aaron.ballman added a comment. This revision is now accepted and ready to land. LGTM! CHANGES SINCE LAST ACTION https://reviews.llvm.org/D134361/new/ https://reviews.llvm.org/D134361 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D134361: [clang][Interp] Fix copy constructors of structs with array members
tbaeder updated this revision to Diff 463522.
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D134361/new/
https://reviews.llvm.org/D134361
Files:
clang/lib/AST/Interp/ByteCodeExprGen.cpp
clang/lib/AST/Interp/ByteCodeExprGen.h
clang/lib/AST/Interp/ByteCodeStmtGen.cpp
clang/test/AST/Interp/records.cpp
Index: clang/test/AST/Interp/records.cpp
===
--- clang/test/AST/Interp/records.cpp
+++ clang/test/AST/Interp/records.cpp
@@ -37,6 +37,9 @@
static_assert(ints.b == 30, "");
static_assert(ints.c, "");
static_assert(ints.getTen() == 10, "");
+static_assert(ints.numbers[0] == 1, "");
+static_assert(ints.numbers[1] == 2, "");
+static_assert(ints.numbers[2] == 3, "");
constexpr const BoolPair = ints.bp;
static_assert(BP.first, "");
@@ -64,11 +67,17 @@
static_assert(ints4.a == (40 * 50), "");
static_assert(ints4.b == 0, "");
static_assert(ints4.c, "");
-
-
-// FIXME: Implement initialization by DeclRefExpr.
-//constexpr Ints ints4 = ints3; TODO
-
+static_assert(ints4.numbers[0] == 1, "");
+static_assert(ints4.numbers[1] == 2, "");
+static_assert(ints4.numbers[2] == 3, "");
+
+constexpr Ints ints5 = ints4;
+static_assert(ints5.a == (40 * 50), "");
+static_assert(ints5.b == 0, "");
+static_assert(ints5.c, "");
+static_assert(ints5.numbers[0] == 1, "");
+static_assert(ints5.numbers[1] == 2, "");
+static_assert(ints5.numbers[2] == 3, "");
struct Ints2 {
Index: clang/lib/AST/Interp/ByteCodeStmtGen.cpp
===
--- clang/lib/AST/Interp/ByteCodeStmtGen.cpp
+++ clang/lib/AST/Interp/ByteCodeStmtGen.cpp
@@ -105,7 +105,7 @@
const Record::Field *F = R->getField(Member);
if (Optional T = this->classify(InitExpr->getType())) {
-if (!this->emitDupPtr(InitExpr))
+if (!this->emitThis(InitExpr))
return false;
if (!this->visit(InitExpr))
@@ -116,7 +116,7 @@
} else {
// Non-primitive case. Get a pointer to the field-to-initialize
// on the stack and call visitInitialzer() for it.
-if (!this->emitDupPtr(InitExpr))
+if (!this->emitThis(InitExpr))
return false;
if (!this->emitGetPtrField(F->Offset, InitExpr))
Index: clang/lib/AST/Interp/ByteCodeExprGen.h
===
--- clang/lib/AST/Interp/ByteCodeExprGen.h
+++ clang/lib/AST/Interp/ByteCodeExprGen.h
@@ -34,6 +34,7 @@
template class VariableScope;
template class DeclScope;
template class OptionScope;
+template class ArrayIndexScope;
/// Compilation context for expressions.
template
@@ -85,6 +86,8 @@
bool VisitConstantExpr(const ConstantExpr *E);
bool VisitUnaryExprOrTypeTraitExpr(const UnaryExprOrTypeTraitExpr *E);
bool VisitMemberExpr(const MemberExpr *E);
+ bool VisitArrayInitIndexExpr(const ArrayInitIndexExpr *E);
+ bool VisitOpaqueValueExpr(const OpaqueValueExpr *E);
protected:
bool visitExpr(const Expr *E) override;
@@ -199,6 +202,7 @@
friend class RecordScope;
friend class DeclScope;
friend class OptionScope;
+ friend class ArrayIndexScope;
/// Emits a zero initializer.
bool visitZeroInitializer(PrimType T, const Expr *E);
@@ -260,7 +264,7 @@
/// Current scope.
VariableScope *VarScope = nullptr;
- /// Current argument index.
+ /// Current argument index. Needed to emit ArrayInitIndexExpr.
llvm::Optional ArrayIndex;
/// Flag indicating if return value is to be discarded.
@@ -362,6 +366,22 @@
}
};
+template class ArrayIndexScope final {
+public:
+ ArrayIndexScope(ByteCodeExprGen *Ctx, uint64_t Index) : Ctx(Ctx) {
+OldArrayIndex = Ctx->ArrayIndex;
+Ctx->ArrayIndex = Index;
+ }
+
+ ~ArrayIndexScope() {
+Ctx->ArrayIndex = OldArrayIndex;
+ }
+
+private:
+ ByteCodeExprGen *Ctx;
+ Optional OldArrayIndex;
+};
+
} // namespace interp
} // namespace clang
Index: clang/lib/AST/Interp/ByteCodeExprGen.cpp
===
--- clang/lib/AST/Interp/ByteCodeExprGen.cpp
+++ clang/lib/AST/Interp/ByteCodeExprGen.cpp
@@ -325,6 +325,18 @@
return false;
}
+template
+bool ByteCodeExprGen::VisitArrayInitIndexExpr(
+const ArrayInitIndexExpr *E) {
+ assert(ArrayIndex);
+ return this->emitConstUint64(*ArrayIndex, E);
+}
+
+template
+bool ByteCodeExprGen::VisitOpaqueValueExpr(const OpaqueValueExpr *E) {
+ return this->visit(E->getSourceExpr());
+}
+
template bool ByteCodeExprGen::discard(const Expr *E) {
OptionScope Scope(this, /*NewDiscardResult=*/true);
return this->Visit(E);
@@ -626,6 +638,32 @@
return true;
} else if (const auto *DIE = dyn_cast(Initializer)) {
return this->visitInitializer(DIE->getExpr());
+ } else if (const auto *AILE = dyn_cast(Initializer)) {
+// 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
[PATCH] D134361: [clang][Interp] Fix copy constructors of structs with array members
tbaeder marked an inline comment as done. tbaeder added inline comments. Comment at: clang/lib/AST/Interp/ByteCodeExprGen.cpp:654-664 + if (!this->emitDupPtr(SubExpr)) +return false; + + if (!this->visit(SubExpr)) +return false; + + if (!this->emitInitElem(*ElemT, I, Initializer)) aaron.ballman wrote: > In all of these cases we're leaving `ArrayIndex` set to `I` instead of > `None`, is that intentional? (Might be worth an RAII object to handle this > sort of thing.) Heh :) Good you notice this as well. Yes, that's something I was worried about. It's not intentional at all. I'll try adding a RAII object. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D134361/new/ https://reviews.llvm.org/D134361 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D134361: [clang][Interp] Fix copy constructors of structs with array members
aaron.ballman added inline comments.
Comment at: clang/lib/AST/Interp/ByteCodeExprGen.cpp:641
return this->visitInitializer(DIE->getExpr());
+ } else if (const auto AILE = dyn_cast(Initializer)) {
+// TODO: This compiles to quite a lot of bytecode if the array is larger.
Comment at: clang/lib/AST/Interp/ByteCodeExprGen.cpp:654-664
+ if (!this->emitDupPtr(SubExpr))
+return false;
+
+ if (!this->visit(SubExpr))
+return false;
+
+ if (!this->emitInitElem(*ElemT, I, Initializer))
In all of these cases we're leaving `ArrayIndex` set to `I` instead of `None`,
is that intentional? (Might be worth an RAII object to handle this sort of
thing.)
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D134361/new/
https://reviews.llvm.org/D134361
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D134361: [clang][Interp] Fix copy constructors of structs with array members
tbaeder added a comment. Ping Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D134361/new/ https://reviews.llvm.org/D134361 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D134361: [clang][Interp] Fix copy constructors of structs with array members
tbaeder created this revision.
tbaeder added reviewers: aaron.ballman, erichkeane, shafik, tahonermann.
Herald added a project: All.
tbaeder requested review of this revision.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.
This implements `ArrayInitLoopExpr`s in initializers and fixes a few issues I
encountered along the way, with copy constructors generally.
Repository:
rG LLVM Github Monorepo
https://reviews.llvm.org/D134361
Files:
clang/lib/AST/Interp/ByteCodeExprGen.cpp
clang/lib/AST/Interp/ByteCodeExprGen.h
clang/lib/AST/Interp/ByteCodeStmtGen.cpp
clang/test/AST/Interp/records.cpp
Index: clang/test/AST/Interp/records.cpp
===
--- clang/test/AST/Interp/records.cpp
+++ clang/test/AST/Interp/records.cpp
@@ -37,6 +37,9 @@
static_assert(ints.b == 30, "");
static_assert(ints.c, "");
static_assert(ints.getTen() == 10, "");
+static_assert(ints.numbers[0] == 1, "");
+static_assert(ints.numbers[1] == 2, "");
+static_assert(ints.numbers[2] == 3, "");
constexpr const BoolPair = ints.bp;
static_assert(BP.first, "");
@@ -64,11 +67,17 @@
static_assert(ints4.a == (40 * 50), "");
static_assert(ints4.b == 0, "");
static_assert(ints4.c, "");
-
-
-// FIXME: Implement initialization by DeclRefExpr.
-//constexpr Ints ints4 = ints3; TODO
-
+static_assert(ints4.numbers[0] == 1, "");
+static_assert(ints4.numbers[1] == 2, "");
+static_assert(ints4.numbers[2] == 3, "");
+
+constexpr Ints ints5 = ints4;
+static_assert(ints5.a == (40 * 50), "");
+static_assert(ints5.b == 0, "");
+static_assert(ints5.c, "");
+static_assert(ints5.numbers[0] == 1, "");
+static_assert(ints5.numbers[1] == 2, "");
+static_assert(ints5.numbers[2] == 3, "");
struct Ints2 {
Index: clang/lib/AST/Interp/ByteCodeStmtGen.cpp
===
--- clang/lib/AST/Interp/ByteCodeStmtGen.cpp
+++ clang/lib/AST/Interp/ByteCodeStmtGen.cpp
@@ -105,7 +105,7 @@
const Record::Field *F = R->getField(Member);
if (Optional T = this->classify(InitExpr->getType())) {
-if (!this->emitDupPtr(InitExpr))
+if (!this->emitThis(InitExpr))
return false;
if (!this->visit(InitExpr))
@@ -116,7 +116,7 @@
} else {
// Non-primitive case. Get a pointer to the field-to-initialize
// on the stack and call visitInitialzer() for it.
-if (!this->emitDupPtr(InitExpr))
+if (!this->emitThis(InitExpr))
return false;
if (!this->emitGetPtrField(F->Offset, InitExpr))
Index: clang/lib/AST/Interp/ByteCodeExprGen.h
===
--- clang/lib/AST/Interp/ByteCodeExprGen.h
+++ clang/lib/AST/Interp/ByteCodeExprGen.h
@@ -85,6 +85,8 @@
bool VisitConstantExpr(const ConstantExpr *E);
bool VisitUnaryExprOrTypeTraitExpr(const UnaryExprOrTypeTraitExpr *E);
bool VisitMemberExpr(const MemberExpr *E);
+ bool VisitArrayInitIndexExpr(const ArrayInitIndexExpr *E);
+ bool VisitOpaqueValueExpr(const OpaqueValueExpr *E);
protected:
bool visitExpr(const Expr *E) override;
@@ -260,7 +262,7 @@
/// Current scope.
VariableScope *VarScope = nullptr;
- /// Current argument index.
+ /// Current argument index. Needed to emit ArrayInitIndexExpr.
llvm::Optional ArrayIndex;
/// Flag indicating if return value is to be discarded.
Index: clang/lib/AST/Interp/ByteCodeExprGen.cpp
===
--- clang/lib/AST/Interp/ByteCodeExprGen.cpp
+++ clang/lib/AST/Interp/ByteCodeExprGen.cpp
@@ -325,6 +325,18 @@
return false;
}
+template
+bool ByteCodeExprGen::VisitArrayInitIndexExpr(
+const ArrayInitIndexExpr *E) {
+ assert(ArrayIndex);
+ return this->emitConstUint64(*ArrayIndex, E);
+}
+
+template
+bool ByteCodeExprGen::VisitOpaqueValueExpr(const OpaqueValueExpr *E) {
+ return this->visit(E->getSourceExpr());
+}
+
template bool ByteCodeExprGen::discard(const Expr *E) {
OptionScope Scope(this, /*NewDiscardResult=*/true);
return this->Visit(E);
@@ -626,6 +638,33 @@
return true;
} else if (const auto *DIE = dyn_cast(Initializer)) {
return this->visitInitializer(DIE->getExpr());
+ } else if (const auto AILE = dyn_cast(Initializer)) {
+// 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 Expr *SubExpr = AILE->getSubExpr();
+size_t Size = AILE->getArraySize().getZExtValue();
+Optional ElemT = classify(SubExpr->getType());
+
+if (!ElemT)
+ return false;
+
+for (size_t I = 0; I != Size; ++I) {
+ ArrayIndex = I;
+ if (!this->emitDupPtr(SubExpr))
+return false;
+
+ if (!this->visit(SubExpr))
+return false;
+
+ if (!this->emitInitElem(*ElemT, I, Initializer))
+return false;
+
+ if
