[AST] Pack the unsigned of SubstTemplateTypeParmPackType into Type

[riccibruno]

Summary

The bit-fields of Type have enough space for the member
unsigned NumArgs of SubstTemplateTypeParmPackType.

[erichkeane]

include/clang/AST/Type.h

1634 ↗

(On Diff #160603)

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.

[riccibruno]

include/clang/AST/Type.h

1634 ↗

(On Diff #160603)

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.

[erichkeane]

include/clang/AST/Type.h

1634 ↗

(On Diff #160603)

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 <whatever>, so it needs to support at least <num-ever>.

[riccibruno]

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.

[erichkeane]

In 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.

[riccibruno]

This comment seems fine. I will also add a similar comment to
TemplateSpecializationTypeBitfields, DependentTemplateSpecializationTypeBitfields
and PackExpansionTypeBitfields since they all 3 have a similar unsigned.

[riccibruno]

updated the comment in SubstTemplateTypeParmPackTypeBitfields

[erichkeane]

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.

cfe/trunk/include/clang/AST/Type.h

class AutoTypeBitfields {
 
     unsigned : NumTypeBits;
 
     /// Was this placeholder type spelled as 'auto', 'decltype(auto)',
     /// or '__auto_type'?  AutoTypeKeyword value.
     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;
 
     unsigned : NumTypeBits;
 
     /// Whether this template specialization type is a substituted type alias.
     unsigned TypeAlias : 1;
 
union {
     AttributedTypeBitfields AttributedTypeBits;
     AutoTypeBitfields AutoTypeBits;
     BuiltinTypeBitfields BuiltinTypeBits;
     FunctionTypeBitfields FunctionTypeBits;
     ObjCObjectTypeBitfields ObjCObjectTypeBits;
     ReferenceTypeBitfields ReferenceTypeBits;
     TypeWithKeywordBitfields TypeWithKeywordBits;
     VectorTypeBitfields VectorTypeBits;
+    SubstTemplateTypeParmPackTypeBitfields SubstTemplateTypeParmPackTypeBits;
     TemplateSpecializationTypeBitfields TemplateSpecializationTypeBits;
     DependentTemplateSpecializationTypeBitfields
       DependentTemplateSpecializationTypeBits;
     PackExpansionTypeBitfields PackExpansionTypeBits;
 
     static_assert(sizeof(TypeBitfields) <= 8,
                   "TypeBitfields is larger than 8 bytes!");
     static_assert(sizeof(ArrayTypeBitfields) <= 8,
union {
     static_assert(sizeof(ObjCObjectTypeBitfields) <= 8,
                   "ObjCObjectTypeBitfields is larger than 8 bytes!");
     static_assert(sizeof(ReferenceTypeBitfields) <= 8,
                   "ReferenceTypeBitfields is larger than 8 bytes!");
     static_assert(sizeof(TypeWithKeywordBitfields) <= 8,
                   "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!");
     static_assert(sizeof(DependentTemplateSpecializationTypeBitfields) <= 8,
                   "DependentTemplateSpecializationTypeBitfields is larger"
                   " than 8 bytes!");
     static_assert(sizeof(PackExpansionTypeBitfields) <= 8,
                   "PackExpansionTypeBitfields is larger than 8 bytes");
class SubstTemplateTypeParmPackType : public Type, public llvm::FoldingSetNode {
 
   /// The original type parameter.
   const TemplateTypeParmType *Replaced;
 
   /// A pointer to the set of template arguments that this
   /// 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 &ArgPack);
 
 public:
   IdentifierInfo *getIdentifier() const { return Replaced->getIdentifier(); }
 
   /// Gets the template parameter that was substituted for.
   const TemplateTypeParmType *getReplacedParameter() const {
     return Replaced;
   }
 
+  unsigned getNumArgs() const {
+    return SubstTemplateTypeParmPackTypeBits.NumArgs;
+  }
+
   bool isSugared() const { return false; }
   QualType desugar() const { return QualType(this, 0); }
 
   TemplateArgument getArgumentPack() const;
 
   void Profile(llvm::FoldingSetNodeID &ID);
   static void Profile(llvm::FoldingSetNodeID &ID,
                       const TemplateTypeParmType *Replaced,

cfe/trunk/lib/AST/Type.cpp

IdentifierInfo *TemplateTypeParmType::getIdentifier() const {
   return isCanonicalUnqualified() ? nullptr : getDecl()->getIdentifier();
 }
 
 SubstTemplateTypeParmPackType::
 SubstTemplateTypeParmPackType(const TemplateTypeParmType *Param,
                               QualType Canon,
                               const TemplateArgument &ArgPack)
     : 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 &ID) {
   Profile(ID, getReplacedParameter(), getArgumentPack());
 }
 
 void SubstTemplateTypeParmPackType::Profile(llvm::FoldingSetNodeID &ID,
                                            const TemplateTypeParmType *Replaced,