[PATCH] D50631: [AST] Stuff more data into FunctionTypeBitfields

[Bruno Ricci via Phabricator via cfe-commits]

riccibruno abandoned this revision.
riccibruno added a comment.

Superseded by https://reviews.llvm.org/D52738.


Repository:
  rC Clang

https://reviews.llvm.org/D50631



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D50631: [AST] Stuff more data into FunctionTypeBitfields

[Bruno Ricci via Phabricator via cfe-commits]

riccibruno planned changes to this revision.
riccibruno added a comment.

I Will put `NumExceptions` in a trailing object. However it seems
that `FunctionProtoType` can be substantially cleaned up by converting
it to use `llvm::TrailingObjects` instead of manually doing the 
casts+arithmetic.

Therefore I plan to first convert `FunctionProtoType` to use 
`llvm::TrailingObjects`
and then resubmit this patch with the appropriate modifications.


Repository:
  rC Clang

https://reviews.llvm.org/D50631



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D50631: [AST] Stuff more data into FunctionTypeBitfields

[John McCall via Phabricator via cfe-commits]

rjmccall added a comment.

Our experience is that we keep adding more complexity to `FunctionType`, so 
it'd be nice if the bits weren't pressed up against the absolute limit.  
Dynamic exception specifications are really common, but only in the 
zero-exceptions case, so as long as we can efficiently represent and detect 
that I think putting the rest of the count out-of-line is fine.


Repository:
  rC Clang

https://reviews.llvm.org/D50631



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D50631: [AST] Stuff more data into FunctionTypeBitfields

[Bruno Ricci via Phabricator via cfe-commits]

riccibruno added a comment.

@rsmith Could you comment on whether limiting the number of types
in a dynamic exception specification to 127 is acceptable ? I had
to steal two bits from `NumExceptions` to make all the fields fit.
If this is a bad idea then a possibility would be to put
`NumExceptions` in a trailing object.


Repository:
  rC Clang

https://reviews.llvm.org/D50631



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D50631: [AST] Stuff more data into FunctionTypeBitfields

[John McCall via Phabricator via cfe-commits]

rjmccall added a comment.

We should absolutely have static assertions to check that these bit-field types 
don't get larger than 32 bits.  A lot of the subclass layouts have been tweaked 
to fit that (packing into the tail padding of `Type` on 64-bit targets), so 
accidentally overflowing to use more bits in the base is going to lead to a lot 
of bloat.


Repository:
  rC Clang

https://reviews.llvm.org/D50631



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D50631: [AST] Stuff more data into FunctionTypeBitfields

[Bruno Ricci via Phabricator via cfe-commits]

riccibruno updated this revision to Diff 160369.
riccibruno marked 5 inline comments as done.
riccibruno edited the summary of this revision.
riccibruno added a comment.

Bumped the number of bits for parameters from 12 to 14,
stealing from NumExceptions. This means that now (unless
limited by something else in clang) the maximum number
of types in a dynamic exception spec is 127, and the
maximum number of function parameters is 16383.


Repository:
  rC Clang

https://reviews.llvm.org/D50631

Files:
  include/clang/AST/Type.h
  lib/AST/Type.cpp

Index: lib/AST/Type.cpp
===
--- lib/AST/Type.cpp
+++ lib/AST/Type.cpp
@@ -2844,24 +2844,23 @@
 FunctionProtoType::FunctionProtoType(QualType result, ArrayRef params,
  QualType canonical,
  const ExtProtoInfo &epi)
-: FunctionType(FunctionProto, result, canonical,
-   result->isDependentType(),
+: FunctionType(FunctionProto, result, canonical, result->isDependentType(),
result->isInstantiationDependentType(),
result->isVariablyModifiedType(),
-   result->containsUnexpandedParameterPack(), epi.ExtInfo),
-  NumParams(params.size()),
-  NumExceptions(epi.ExceptionSpec.Exceptions.size()),
-  ExceptionSpecType(epi.ExceptionSpec.Type),
-  HasExtParameterInfos(epi.ExtParameterInfos != nullptr),
-  Variadic(epi.Variadic), HasTrailingReturn(epi.HasTrailingReturn) {
-  assert(NumParams == params.size() && "function has too many parameters");
-
+   result->containsUnexpandedParameterPack(), epi.ExtInfo) {
   FunctionTypeBits.TypeQuals = epi.TypeQuals;
   FunctionTypeBits.RefQualifier = epi.RefQualifier;
+  FunctionTypeBits.NumParams = params.size();
+  assert(getNumParams() == params.size() && "function w. too many parameters!");
+  FunctionTypeBits.NumExceptions = epi.ExceptionSpec.Exceptions.size();
+  FunctionTypeBits.ExceptionSpecType = epi.ExceptionSpec.Type;
+  FunctionTypeBits.HasExtParameterInfos = !!epi.ExtParameterInfos;
+  FunctionTypeBits.Variadic = epi.Variadic;
+  FunctionTypeBits.HasTrailingReturn = epi.HasTrailingReturn;
 
   // Fill in the trailing argument array.
   auto *argSlot = reinterpret_cast(this+1);
-  for (unsigned i = 0; i != NumParams; ++i) {
+  for (unsigned i = 0; i != getNumParams(); ++i) {
 if (params[i]->isDependentType())
   setDependent();
 else if (params[i]->isInstantiationDependentType())
@@ -2875,7 +2874,7 @@
 
   if (getExceptionSpecType() == EST_Dynamic) {
 // Fill in the exception array.
-QualType *exnSlot = argSlot + NumParams;
+QualType *exnSlot = argSlot + getNumParams();
 unsigned I = 0;
 for (QualType ExceptionType : epi.ExceptionSpec.Exceptions) {
   // Note that, before C++17, a dependent exception specification does
@@ -2895,7 +2894,7 @@
epi.ExceptionSpec.NoexceptExpr->isValueDependent());
 
 // Store the noexcept expression and context.
-auto **noexSlot = reinterpret_cast(argSlot + NumParams);
+auto **noexSlot = reinterpret_cast(argSlot + getNumParams());
 *noexSlot = epi.ExceptionSpec.NoexceptExpr;
 
 if (epi.ExceptionSpec.NoexceptExpr->isValueDependent() ||
@@ -2907,15 +2906,15 @@
   } else if (getExceptionSpecType() == EST_Uninstantiated) {
 // Store the function decl from which we will resolve our
 // exception specification.
-auto **slot = reinterpret_cast(argSlot + NumParams);
+auto **slot = reinterpret_cast(argSlot + getNumParams());
 slot[0] = epi.ExceptionSpec.SourceDecl;
 slot[1] = epi.ExceptionSpec.SourceTemplate;
 // This exception specification doesn't make the type dependent, because
 // it's not instantiated as part of instantiating the type.
   } else if (getExceptionSpecType() == EST_Unevaluated) {
 // Store the function decl from which we will resolve our
 // exception specification.
-auto **slot = reinterpret_cast(argSlot + NumParams);
+auto **slot = reinterpret_cast(argSlot + getNumParams());
 slot[0] = epi.ExceptionSpec.SourceDecl;
   }
 
@@ -2935,7 +2934,7 @@
   if (epi.ExtParameterInfos) {
 auto *extParamInfos =
   const_cast(getExtParameterInfosBuffer());
-for (unsigned i = 0; i != NumParams; ++i)
+for (unsigned i = 0; i != getNumParams(); ++i)
   extParamInfos[i] = epi.ExtParameterInfos[i];
   }
 }
@@ -2981,7 +2980,7 @@
   case EST_Dynamic:
 // A dynamic exception specification is throwing unless every exception
 // type is an (unexpanded) pack expansion type.
-for (unsigned I = 0, N = NumExceptions; I != N; ++I)
+for (unsigned I = 0; I != getNumExceptions(); ++I)
   if (!getExceptionType(I)->getAs())
 return CT_Can;
 return CT_Dependent;
@@ -3056,7 +3055,8 @@
 
 void FunctionProtoType::Profile(llvm::FoldingSetNodeID &ID,
 const ASTContext &Ctx) {
-  P

[PATCH] D50631: [AST] Stuff more data into FunctionTypeBitfields

[Bruno Ricci via Phabricator via cfe-commits]

riccibruno added inline comments.



Comment at: include/clang/AST/Type.h:1530
+/// The number of types in the exception spec, if any.
+unsigned NumExceptions : 9;
+

erichkeane wrote:
> IMO (and @rsmith should respond here instead), if we were looking to steal 
> bits from anywhere, this would be it.  Exception-specifications that use 
> types ares are deprecated in the language and I'm not sure anyone ever used 
> them anyway.
Yes this seems a good idea. We could steal 2 bits (or more) from
here to restore the 14 bits in NumParams. I can't imagine that
someone will miss not being able to specify > 512 types
in an exception specification.



Comment at: lib/AST/Type.cpp:2863
   auto *argSlot = reinterpret_cast(this+1);
-  for (unsigned i = 0; i != NumParams; ++i) {
+  for (unsigned i = 0, N = getNumParams(); i != N; ++i) {
 if (params[i]->isDependentType())

erichkeane wrote:
> I would be unbelievably surprised if this change is worth-while.  I can't see 
> a situation where this getNumParams call doesn't get inlined.
It was just for consistency with the other changes,
but perhaps it do not belongs to this patch.


Repository:
  rC Clang

https://reviews.llvm.org/D50631



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D50631: [AST] Stuff more data into FunctionTypeBitfields

[Erich Keane via Phabricator via cfe-commits]

erichkeane added a subscriber: rsmith.
erichkeane added inline comments.



Comment at: include/clang/AST/Type.h:1527
+/// The number of parameters this function has, not counting '...'.
+unsigned NumParams : 12;
+

This concerns me a bit with variadic templates.  I realize implimits says 256 
but IMO variadic templates makes this a fairly easy limit to hit.  I guess that 
4096 is probably sufficient, though I'd like to hear someone else's opinion.



Comment at: include/clang/AST/Type.h:1530
+/// The number of types in the exception spec, if any.
+unsigned NumExceptions : 9;
+

IMO (and @rsmith should respond here instead), if we were looking to steal bits 
from anywhere, this would be it.  Exception-specifications that use types ares 
are deprecated in the language and I'm not sure anyone ever used them anyway.



Comment at: lib/AST/Type.cpp:2863
   auto *argSlot = reinterpret_cast(this+1);
-  for (unsigned i = 0; i != NumParams; ++i) {
+  for (unsigned i = 0, N = getNumParams(); i != N; ++i) {
 if (params[i]->isDependentType())

I would be unbelievably surprised if this change is worth-while.  I can't see a 
situation where this getNumParams call doesn't get inlined.


Repository:
  rC Clang

https://reviews.llvm.org/D50631



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D50631: [AST] Stuff more data into FunctionTypeBitfields

[Bruno Ricci via Phabricator via cfe-commits]

riccibruno created this revision.
riccibruno added reviewers: erichkeane, rjmccall.
riccibruno added a project: clang.
Herald added a subscriber: cfe-commits.

Since FunctionTypeBitfields is already > 32 bits wide we might
as well stuff the remaining bits from FunctionProtoType into it.

The patch is very mechanical, but changes the maximum number of
parameters from 2^15-1 to 2^12-1 to fit into FunctionTypeBitfields.
This seems still large enough, and in any case according to the
annex B of the C++ standard we only need to be able to accept
256 parameters.

Also the member `unsigned RefQualifier : 2;` is moved in 
FunctionTypeBitfields. This is intentional and done to avoid
the unsigned boundary.


Repository:
  rC Clang

https://reviews.llvm.org/D50631

Files:
  include/clang/AST/Type.h
  lib/AST/Type.cpp

Index: lib/AST/Type.cpp
===
--- lib/AST/Type.cpp
+++ lib/AST/Type.cpp
@@ -2844,24 +2844,23 @@
 FunctionProtoType::FunctionProtoType(QualType result, ArrayRef params,
  QualType canonical,
  const ExtProtoInfo &epi)
-: FunctionType(FunctionProto, result, canonical,
-   result->isDependentType(),
+: FunctionType(FunctionProto, result, canonical, result->isDependentType(),
result->isInstantiationDependentType(),
result->isVariablyModifiedType(),
-   result->containsUnexpandedParameterPack(), epi.ExtInfo),
-  NumParams(params.size()),
-  NumExceptions(epi.ExceptionSpec.Exceptions.size()),
-  ExceptionSpecType(epi.ExceptionSpec.Type),
-  HasExtParameterInfos(epi.ExtParameterInfos != nullptr),
-  Variadic(epi.Variadic), HasTrailingReturn(epi.HasTrailingReturn) {
-  assert(NumParams == params.size() && "function has too many parameters");
-
+   result->containsUnexpandedParameterPack(), epi.ExtInfo) {
   FunctionTypeBits.TypeQuals = epi.TypeQuals;
   FunctionTypeBits.RefQualifier = epi.RefQualifier;
+  FunctionTypeBits.NumParams = params.size();
+  assert(getNumParams() == params.size() && "function w. too many parameters!");
+  FunctionTypeBits.NumExceptions = epi.ExceptionSpec.Exceptions.size();
+  FunctionTypeBits.ExceptionSpecType = epi.ExceptionSpec.Type;
+  FunctionTypeBits.HasExtParameterInfos = !!epi.ExtParameterInfos;
+  FunctionTypeBits.Variadic = epi.Variadic;
+  FunctionTypeBits.HasTrailingReturn = epi.HasTrailingReturn;
 
   // Fill in the trailing argument array.
   auto *argSlot = reinterpret_cast(this+1);
-  for (unsigned i = 0; i != NumParams; ++i) {
+  for (unsigned i = 0, N = getNumParams(); i != N; ++i) {
 if (params[i]->isDependentType())
   setDependent();
 else if (params[i]->isInstantiationDependentType())
@@ -2875,7 +2874,7 @@
 
   if (getExceptionSpecType() == EST_Dynamic) {
 // Fill in the exception array.
-QualType *exnSlot = argSlot + NumParams;
+QualType *exnSlot = argSlot + getNumParams();
 unsigned I = 0;
 for (QualType ExceptionType : epi.ExceptionSpec.Exceptions) {
   // Note that, before C++17, a dependent exception specification does
@@ -2895,7 +2894,7 @@
epi.ExceptionSpec.NoexceptExpr->isValueDependent());
 
 // Store the noexcept expression and context.
-auto **noexSlot = reinterpret_cast(argSlot + NumParams);
+auto **noexSlot = reinterpret_cast(argSlot + getNumParams());
 *noexSlot = epi.ExceptionSpec.NoexceptExpr;
 
 if (epi.ExceptionSpec.NoexceptExpr->isValueDependent() ||
@@ -2907,15 +2906,15 @@
   } else if (getExceptionSpecType() == EST_Uninstantiated) {
 // Store the function decl from which we will resolve our
 // exception specification.
-auto **slot = reinterpret_cast(argSlot + NumParams);
+auto **slot = reinterpret_cast(argSlot + getNumParams());
 slot[0] = epi.ExceptionSpec.SourceDecl;
 slot[1] = epi.ExceptionSpec.SourceTemplate;
 // This exception specification doesn't make the type dependent, because
 // it's not instantiated as part of instantiating the type.
   } else if (getExceptionSpecType() == EST_Unevaluated) {
 // Store the function decl from which we will resolve our
 // exception specification.
-auto **slot = reinterpret_cast(argSlot + NumParams);
+auto **slot = reinterpret_cast(argSlot + getNumParams());
 slot[0] = epi.ExceptionSpec.SourceDecl;
   }
 
@@ -2935,7 +2934,7 @@
   if (epi.ExtParameterInfos) {
 auto *extParamInfos =
   const_cast(getExtParameterInfosBuffer());
-for (unsigned i = 0; i != NumParams; ++i)
+for (unsigned i = 0, N = getNumParams(); i != N; ++i)
   extParamInfos[i] = epi.ExtParameterInfos[i];
   }
 }
@@ -2981,7 +2980,7 @@
   case EST_Dynamic:
 // A dynamic exception specification is throwing unless every exception
 // type is an (unexpanded) pack expansion type.
-for (unsigned I = 0, N = NumExceptions; I != N; ++I)
+for (unsig