[PATCH] D50713: [AST] Pack the unsigned of SubstTemplateTypeParmPackType into Type
This revision was automatically updated to reflect the committed changes.
Closed by commit rL339861: [AST] Pack the unsigned of
SubstTemplateTypeParmPackType into Type (authored by brunoricci, committed by ).
Herald added a subscriber: llvm-commits.
Changed prior to commit:
https://reviews.llvm.org/D50713?vs=160795=160991#toc
Repository:
rL LLVM
https://reviews.llvm.org/D50713
Files:
cfe/trunk/include/clang/AST/Type.h
cfe/trunk/lib/AST/Type.cpp
Index: cfe/trunk/lib/AST/Type.cpp
===
--- cfe/trunk/lib/AST/Type.cpp
+++ cfe/trunk/lib/AST/Type.cpp
@@ -3285,11 +3285,12 @@
QualType Canon,
const TemplateArgument )
: Type(SubstTemplateTypeParmPack, Canon, true, true, false, true),
- Replaced(Param),
- Arguments(ArgPack.pack_begin()), NumArguments(ArgPack.pack_size()) {}
+ Replaced(Param), Arguments(ArgPack.pack_begin()) {
+ SubstTemplateTypeParmPackTypeBits.NumArgs = ArgPack.pack_size();
+}
TemplateArgument SubstTemplateTypeParmPackType::getArgumentPack() const {
- return TemplateArgument(llvm::makeArrayRef(Arguments, NumArguments));
+ return TemplateArgument(llvm::makeArrayRef(Arguments, getNumArgs()));
}
void SubstTemplateTypeParmPackType::Profile(llvm::FoldingSetNodeID ) {
Index: cfe/trunk/include/clang/AST/Type.h
===
--- cfe/trunk/include/clang/AST/Type.h
+++ cfe/trunk/include/clang/AST/Type.h
@@ -1608,6 +1608,21 @@
unsigned Keyword : 2;
};
+ class SubstTemplateTypeParmPackTypeBitfields {
+friend class SubstTemplateTypeParmPackType;
+
+unsigned : NumTypeBits;
+
+/// The number of template arguments in \c Arguments, which is
+/// expected to be able to hold at least 1024 according to [implimits].
+/// However as this limit is somewhat easy to hit with template
+/// metaprogramming we'd prefer to keep it as large as possible.
+/// At the moment it has been left as a non-bitfield since this type
+/// safely fits in 64 bits as an unsigned, so there is no reason to
+/// introduce the performance impact of a bitfield.
+unsigned NumArgs;
+ };
+
class TemplateSpecializationTypeBitfields {
friend class TemplateSpecializationType;
@@ -1672,6 +1687,7 @@
ReferenceTypeBitfields ReferenceTypeBits;
TypeWithKeywordBitfields TypeWithKeywordBits;
VectorTypeBitfields VectorTypeBits;
+SubstTemplateTypeParmPackTypeBitfields SubstTemplateTypeParmPackTypeBits;
TemplateSpecializationTypeBitfields TemplateSpecializationTypeBits;
DependentTemplateSpecializationTypeBitfields
DependentTemplateSpecializationTypeBits;
@@ -1697,6 +1713,9 @@
"TypeWithKeywordBitfields is larger than 8 bytes!");
static_assert(sizeof(VectorTypeBitfields) <= 8,
"VectorTypeBitfields is larger than 8 bytes!");
+static_assert(sizeof(SubstTemplateTypeParmPackTypeBitfields) <= 8,
+ "SubstTemplateTypeParmPackTypeBitfields is larger"
+ " than 8 bytes!");
static_assert(sizeof(TemplateSpecializationTypeBitfields) <= 8,
"TemplateSpecializationTypeBitfields is larger"
" than 8 bytes!");
@@ -4551,9 +4570,6 @@
/// parameter pack is instantiated with.
const TemplateArgument *Arguments;
- /// The number of template arguments in \c Arguments.
- unsigned NumArguments;
-
SubstTemplateTypeParmPackType(const TemplateTypeParmType *Param,
QualType Canon,
const TemplateArgument );
@@ -4566,6 +4582,10 @@
return Replaced;
}
+ unsigned getNumArgs() const {
+return SubstTemplateTypeParmPackTypeBits.NumArgs;
+ }
+
bool isSugared() const { return false; }
QualType desugar() const { return QualType(this, 0); }
Index: cfe/trunk/lib/AST/Type.cpp
===
--- cfe/trunk/lib/AST/Type.cpp
+++ cfe/trunk/lib/AST/Type.cpp
@@ -3285,11 +3285,12 @@
QualType Canon,
const TemplateArgument )
: Type(SubstTemplateTypeParmPack, Canon, true, true, false, true),
- Replaced(Param),
- Arguments(ArgPack.pack_begin()), NumArguments(ArgPack.pack_size()) {}
+ Replaced(Param), Arguments(ArgPack.pack_begin()) {
+ SubstTemplateTypeParmPackTypeBits.NumArgs = ArgPack.pack_size();
+}
TemplateArgument SubstTemplateTypeParmPackType::getArgumentPack() const {
- return TemplateArgument(llvm::makeArrayRef(Arguments, NumArguments));
+ return TemplateArgument(llvm::makeArrayRef(Arguments, getNumArgs()));
}
void SubstTemplateTypeParmPackType::Profile(llvm::FoldingSetNodeID ) {
Index: cfe/trunk/include/clang/AST/Type.h
===
--- cfe/trunk/include/clang/AST/Type.h
+++
[PATCH] D50713: [AST] Pack the unsigned of SubstTemplateTypeParmPackType into Type
erichkeane accepted this revision. erichkeane added a comment. This revision is now accepted and ready to land. Sorry about the long discussion on the comment... These bitfields are just hell on the next guy through if he doesn't have the context/knowledge of what are appropriate sizes. Repository: rC Clang https://reviews.llvm.org/D50713 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D50713: [AST] Pack the unsigned of SubstTemplateTypeParmPackType into Type
riccibruno updated this revision to Diff 160795.
riccibruno added a comment.
updated the comment in SubstTemplateTypeParmPackTypeBitfields
Repository:
rC Clang
https://reviews.llvm.org/D50713
Files:
include/clang/AST/Type.h
lib/AST/Type.cpp
Index: lib/AST/Type.cpp
===
--- lib/AST/Type.cpp
+++ lib/AST/Type.cpp
@@ -3285,11 +3285,12 @@
QualType Canon,
const TemplateArgument )
: Type(SubstTemplateTypeParmPack, Canon, true, true, false, true),
- Replaced(Param),
- Arguments(ArgPack.pack_begin()), NumArguments(ArgPack.pack_size()) {}
+ Replaced(Param), Arguments(ArgPack.pack_begin()) {
+ SubstTemplateTypeParmPackTypeBits.NumArgs = ArgPack.pack_size();
+}
TemplateArgument SubstTemplateTypeParmPackType::getArgumentPack() const {
- return TemplateArgument(llvm::makeArrayRef(Arguments, NumArguments));
+ return TemplateArgument(llvm::makeArrayRef(Arguments, getNumArgs()));
}
void SubstTemplateTypeParmPackType::Profile(llvm::FoldingSetNodeID ) {
Index: include/clang/AST/Type.h
===
--- include/clang/AST/Type.h
+++ include/clang/AST/Type.h
@@ -1626,6 +1626,21 @@
unsigned Keyword : 2;
};
+ class SubstTemplateTypeParmPackTypeBitfields {
+friend class SubstTemplateTypeParmPackType;
+
+unsigned : NumTypeBits;
+
+/// The number of template arguments in \c Arguments, which is
+/// expected to be able to hold at least 1024 according to [implimits].
+/// However as this limit is somewhat easy to hit with template
+/// metaprogramming we'd prefer to keep it as large as possible.
+/// At the moment it has been left as a non-bitfield since this type
+/// safely fits in 64 bits as an unsigned, so there is no reason to
+/// introduce the performance impact of a bitfield.
+unsigned NumArgs;
+ };
+
class TemplateSpecializationTypeBitfields {
friend class TemplateSpecializationType;
@@ -1690,6 +1705,7 @@
ReferenceTypeBitfields ReferenceTypeBits;
TypeWithKeywordBitfields TypeWithKeywordBits;
VectorTypeBitfields VectorTypeBits;
+SubstTemplateTypeParmPackTypeBitfields SubstTemplateTypeParmPackTypeBits;
TemplateSpecializationTypeBitfields TemplateSpecializationTypeBits;
DependentTemplateSpecializationTypeBitfields
DependentTemplateSpecializationTypeBits;
@@ -1715,6 +1731,9 @@
"TypeWithKeywordBitfields is larger than 8 bytes!");
static_assert(sizeof(VectorTypeBitfields) <= 8,
"VectorTypeBitfields is larger than 8 bytes!");
+static_assert(sizeof(SubstTemplateTypeParmPackTypeBitfields) <= 8,
+ "SubstTemplateTypeParmPackTypeBitfields is larger"
+ " than 8 bytes!");
static_assert(sizeof(TemplateSpecializationTypeBitfields) <= 8,
"TemplateSpecializationTypeBitfields is larger"
" than 8 bytes!");
@@ -4555,9 +4574,6 @@
/// parameter pack is instantiated with.
const TemplateArgument *Arguments;
- /// The number of template arguments in \c Arguments.
- unsigned NumArguments;
-
SubstTemplateTypeParmPackType(const TemplateTypeParmType *Param,
QualType Canon,
const TemplateArgument );
@@ -4570,6 +4586,10 @@
return Replaced;
}
+ unsigned getNumArgs() const {
+return SubstTemplateTypeParmPackTypeBits.NumArgs;
+ }
+
bool isSugared() const { return false; }
QualType desugar() const { return QualType(this, 0); }
Index: lib/AST/Type.cpp
===
--- lib/AST/Type.cpp
+++ lib/AST/Type.cpp
@@ -3285,11 +3285,12 @@
QualType Canon,
const TemplateArgument )
: Type(SubstTemplateTypeParmPack, Canon, true, true, false, true),
- Replaced(Param),
- Arguments(ArgPack.pack_begin()), NumArguments(ArgPack.pack_size()) {}
+ Replaced(Param), Arguments(ArgPack.pack_begin()) {
+ SubstTemplateTypeParmPackTypeBits.NumArgs = ArgPack.pack_size();
+}
TemplateArgument SubstTemplateTypeParmPackType::getArgumentPack() const {
- return TemplateArgument(llvm::makeArrayRef(Arguments, NumArguments));
+ return TemplateArgument(llvm::makeArrayRef(Arguments, getNumArgs()));
}
void SubstTemplateTypeParmPackType::Profile(llvm::FoldingSetNodeID ) {
Index: include/clang/AST/Type.h
===
--- include/clang/AST/Type.h
+++ include/clang/AST/Type.h
@@ -1626,6 +1626,21 @@
unsigned Keyword : 2;
};
+ class SubstTemplateTypeParmPackTypeBitfields {
+friend class SubstTemplateTypeParmPackType;
+
+unsigned : NumTypeBits;
+
+/// The number of template arguments in \c Arguments, which is
+/// expected to be able to hold at
[PATCH] D50713: [AST] Pack the unsigned of SubstTemplateTypeParmPackType into Type
riccibruno added a comment. This comment seems fine. I will also add a similar comment to `TemplateSpecializationTypeBitfields`, `DependentTemplateSpecializationTypeBitfields` and `PackExpansionTypeBitfields` since they all 3 have a similar `unsigned`. Repository: rC Clang https://reviews.llvm.org/D50713 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D50713: [AST] Pack the unsigned of SubstTemplateTypeParmPackType into Type
erichkeane added a comment. In https://reviews.llvm.org/D50713#1200564, @riccibruno wrote: > The thing is that I have no idea what is the minimum number > of bits required. It was originally an unsigned but I suspect that > 32 bits are not needed. Looking at [implimits] I see > > Template arguments in a template declaration [1 024]. > Recursively nested template instantiations, including substitution during > template argument deduc- > tion (17.8.2) [1 024]. > > > But 1024 seems to be too small and easily exceeded by a doing a bit > of template meta-programming. I suspect a minor rephrase of this comment would be more than sufficient for me: "This field corresponds to the number of template arguments, which is expected to be at least 1024 according to [implimits]. However, as this limit is somewhat easy to hit with template metaprogramming we'd prefer to keep it as large as possible. At the moment it has been left as a non-bitfield since this type safely fits in 64 bits as an unsigned, so there is no reason to introduce the performance impact of a bitfield." I'd suspect you/someone will bikeshed that, but I think it gets the point across that I want to make sure is there. Repository: rC Clang https://reviews.llvm.org/D50713 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D50713: [AST] Pack the unsigned of SubstTemplateTypeParmPackType into Type
riccibruno added a comment. The thing is that I have no idea what is the minimum number of bits required. It was originally an unsigned but I suspect that 32 bits are not needed. Looking at [implimits] I see Template arguments in a template declaration [1 024]. Recursively nested template instantiations, including substitution during template argument deduc- tion (17.8.2) [1 024]. But 1024 seems to be too small and easily exceeded by a doing a bit of template meta-programming. Repository: rC Clang https://reviews.llvm.org/D50713 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D50713: [AST] Pack the unsigned of SubstTemplateTypeParmPackType into Type
erichkeane added inline comments. Comment at: include/clang/AST/Type.h:1634 + +/// The number of template arguments in \c Arguments. +/// Intentionally not a bitfield since we have plenty of space left. riccibruno wrote: > erichkeane wrote: > > I would love it if we commented what a reasonable number for this is. > > Additionally, we should probably make it 64 - NumTypeBits or something, to > > ensure that if NumTypeBits gets too large that we don't blow 64 bits here > > as well as communicate that it isn't necessary to keep this 32 bits. > The reason I did not do something like this is that > there is a penalty to access bit-fields due to the various > shifts and masks. IIRC I was actually able to measure it after > I went over each of these classes and only kept bit-fields > when strictly necessary. (however it is hard to be sure > since it was close to the noise of my measurement, and the > difference could be from totally unrelated sources). > In any case it is quite small (maybe ~0.1% in total, not just > from this single case) and IMHO not worth systematically > bothering about it but in this case why pay the penalty if > we can avoid it. > > If `NumTypeBits` gets too large then this will be detected by the > the static_asserts and the person who did the modification will > have to convert `NumArgs` to a bit-field. In any case this will at least > also blow up `FunctionTypeBitfields`, `VectorTypeBitfields`, > `AttributedTypeBitfields`, `SubstTemplateTypeParmPackTypeBitfields`, > `TemplateSpecializationTypeBitfields`, > `DependentTemplateSpecializationTypeBitfields`, > and `PackExpansionTypeBitfields`. > > Also I do not think that doing > `unsigned NumArgs : 64 - NumTypeBits;` will work because > since `NumTypeBits == 18` this will be > `unsigned NumArgs : 46;` and this will not pack nicely. > > Given how many things will not work when `NumTypeBits > 32` > It seems to be that it is not worth converting it to a bit-field. > However a comment indicating how many bits are actually needed > would definitely be a good addition IMO. On this point I have > absolutely no idea and probably @rsmith should comment on this. My biggest fear here is basically that a future programmer coming in is going to figure that NumArgs REQUIRES 32 bits and try to start the discussion as to whether we can blow this bitfields up. A comment saying most of what you said above, plus a "This corresponds to implimits , so it needs to support at least . Repository: rC Clang https://reviews.llvm.org/D50713 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D50713: [AST] Pack the unsigned of SubstTemplateTypeParmPackType into Type
riccibruno added inline comments. Comment at: include/clang/AST/Type.h:1634 + +/// The number of template arguments in \c Arguments. +/// Intentionally not a bitfield since we have plenty of space left. erichkeane wrote: > I would love it if we commented what a reasonable number for this is. > Additionally, we should probably make it 64 - NumTypeBits or something, to > ensure that if NumTypeBits gets too large that we don't blow 64 bits here as > well as communicate that it isn't necessary to keep this 32 bits. The reason I did not do something like this is that there is a penalty to access bit-fields due to the various shifts and masks. IIRC I was actually able to measure it after I went over each of these classes and only kept bit-fields when strictly necessary. (however it is hard to be sure since it was close to the noise of my measurement, and the difference could be from totally unrelated sources). In any case it is quite small (maybe ~0.1% in total, not just from this single case) and IMHO not worth systematically bothering about it but in this case why pay the penalty if we can avoid it. If `NumTypeBits` gets too large then this will be detected by the the static_asserts and the person who did the modification will have to convert `NumArgs` to a bit-field. In any case this will at least also blow up `FunctionTypeBitfields`, `VectorTypeBitfields`, `AttributedTypeBitfields`, `SubstTemplateTypeParmPackTypeBitfields`, `TemplateSpecializationTypeBitfields`, `DependentTemplateSpecializationTypeBitfields`, and `PackExpansionTypeBitfields`. Also I do not think that doing `unsigned NumArgs : 64 - NumTypeBits;` will work because since `NumTypeBits == 18` this will be `unsigned NumArgs : 46;` and this will not pack nicely. Given how many things will not work when `NumTypeBits > 32` It seems to be that it is not worth converting it to a bit-field. However a comment indicating how many bits are actually needed would definitely be a good addition IMO. On this point I have absolutely no idea and probably @rsmith should comment on this. Repository: rC Clang https://reviews.llvm.org/D50713 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D50713: [AST] Pack the unsigned of SubstTemplateTypeParmPackType into Type
erichkeane added inline comments. Comment at: include/clang/AST/Type.h:1634 + +/// The number of template arguments in \c Arguments. +/// Intentionally not a bitfield since we have plenty of space left. I would love it if we commented what a reasonable number for this is. Additionally, we should probably make it 64 - NumTypeBits or something, to ensure that if NumTypeBits gets too large that we don't blow 64 bits here as well as communicate that it isn't necessary to keep this 32 bits. Repository: rC Clang https://reviews.llvm.org/D50713 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D50713: [AST] Pack the unsigned of SubstTemplateTypeParmPackType into Type
riccibruno created this revision.
riccibruno added reviewers: rsmith, rjmccall, erichkeane.
riccibruno added a project: clang.
Herald added a reviewer: javed.absar.
Herald added subscribers: cfe-commits, chrib, kristof.beyls.
The bit-fields of `Type` have enough space for the member
`unsigned NumArgs` of SubstTemplateTypeParmPackType.
Repository:
rC Clang
https://reviews.llvm.org/D50713
Files:
include/clang/AST/Type.h
lib/AST/Type.cpp
Index: lib/AST/Type.cpp
===
--- lib/AST/Type.cpp
+++ lib/AST/Type.cpp
@@ -3285,11 +3285,12 @@
QualType Canon,
const TemplateArgument )
: Type(SubstTemplateTypeParmPack, Canon, true, true, false, true),
- Replaced(Param),
- Arguments(ArgPack.pack_begin()), NumArguments(ArgPack.pack_size()) {}
+ Replaced(Param), Arguments(ArgPack.pack_begin()) {
+ SubstTemplateTypeParmPackTypeBits.NumArgs = ArgPack.pack_size();
+}
TemplateArgument SubstTemplateTypeParmPackType::getArgumentPack() const {
- return TemplateArgument(llvm::makeArrayRef(Arguments, NumArguments));
+ return TemplateArgument(llvm::makeArrayRef(Arguments, getNumArgs()));
}
void SubstTemplateTypeParmPackType::Profile(llvm::FoldingSetNodeID ) {
Index: include/clang/AST/Type.h
===
--- include/clang/AST/Type.h
+++ include/clang/AST/Type.h
@@ -1626,6 +1626,16 @@
unsigned Keyword : 2;
};
+ class SubstTemplateTypeParmPackTypeBitfields {
+friend class SubstTemplateTypeParmPackType;
+
+unsigned : NumTypeBits;
+
+/// The number of template arguments in \c Arguments.
+/// Intentionally not a bitfield since we have plenty of space left.
+unsigned NumArgs;
+ };
+
class TemplateSpecializationTypeBitfields {
friend class TemplateSpecializationType;
@@ -1678,6 +1688,7 @@
ReferenceTypeBitfields ReferenceTypeBits;
TypeWithKeywordBitfields TypeWithKeywordBits;
VectorTypeBitfields VectorTypeBits;
+SubstTemplateTypeParmPackTypeBitfields SubstTemplateTypeParmPackTypeBits;
TemplateSpecializationTypeBitfields TemplateSpecializationTypeBits;
DependentTemplateSpecializationTypeBitfields
DependentTemplateSpecializationTypeBits;
@@ -1703,6 +1714,9 @@
"TypeWithKeywordBitfields is larger than 8 bytes!");
static_assert(sizeof(VectorTypeBitfields) <= 8,
"VectorTypeBitfields is larger than 8 bytes!");
+static_assert(sizeof(SubstTemplateTypeParmPackTypeBitfields) <= 8,
+ "SubstTemplateTypeParmPackTypeBitfields is larger"
+ " than 8 bytes!");
static_assert(sizeof(TemplateSpecializationTypeBitfields) <= 8,
"TemplateSpecializationTypeBitfields is larger"
" than 8 bytes!");
@@ -4543,9 +4557,6 @@
/// parameter pack is instantiated with.
const TemplateArgument *Arguments;
- /// The number of template arguments in \c Arguments.
- unsigned NumArguments;
-
SubstTemplateTypeParmPackType(const TemplateTypeParmType *Param,
QualType Canon,
const TemplateArgument );
@@ -4558,6 +4569,10 @@
return Replaced;
}
+ unsigned getNumArgs() const {
+return SubstTemplateTypeParmPackTypeBits.NumArgs;
+ }
+
bool isSugared() const { return false; }
QualType desugar() const { return QualType(this, 0); }
Index: lib/AST/Type.cpp
===
--- lib/AST/Type.cpp
+++ lib/AST/Type.cpp
@@ -3285,11 +3285,12 @@
QualType Canon,
const TemplateArgument )
: Type(SubstTemplateTypeParmPack, Canon, true, true, false, true),
- Replaced(Param),
- Arguments(ArgPack.pack_begin()), NumArguments(ArgPack.pack_size()) {}
+ Replaced(Param), Arguments(ArgPack.pack_begin()) {
+ SubstTemplateTypeParmPackTypeBits.NumArgs = ArgPack.pack_size();
+}
TemplateArgument SubstTemplateTypeParmPackType::getArgumentPack() const {
- return TemplateArgument(llvm::makeArrayRef(Arguments, NumArguments));
+ return TemplateArgument(llvm::makeArrayRef(Arguments, getNumArgs()));
}
void SubstTemplateTypeParmPackType::Profile(llvm::FoldingSetNodeID ) {
Index: include/clang/AST/Type.h
===
--- include/clang/AST/Type.h
+++ include/clang/AST/Type.h
@@ -1626,6 +1626,16 @@
unsigned Keyword : 2;
};
+ class SubstTemplateTypeParmPackTypeBitfields {
+friend class SubstTemplateTypeParmPackType;
+
+unsigned : NumTypeBits;
+
+/// The number of template arguments in \c Arguments.
+/// Intentionally not a bitfield since we have plenty of space left.
+unsigned NumArgs;
+ };
+
class TemplateSpecializationTypeBitfields {
friend class TemplateSpecializationType;
@@
