[PATCH] D52267: [AST] Various optimizations + refactoring in DeclarationName(Table)

[Phabricator via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rC342729: [AST] Various optimizations + refactoring in 
DeclarationName(Table) (authored by brunoricci, committed by ).

Repository:
  rC Clang

https://reviews.llvm.org/D52267

Files:
  include/clang/AST/DeclarationName.h
  include/clang/Basic/IdentifierTable.h
  lib/AST/DeclarationName.cpp
  lib/Basic/IdentifierTable.cpp
  lib/Sema/IdentifierResolver.cpp
  lib/Serialization/ASTReader.cpp
  lib/Serialization/ASTWriter.cpp

Index: include/clang/AST/DeclarationName.h
===
--- include/clang/AST/DeclarationName.h
+++ include/clang/AST/DeclarationName.h
@@ -17,6 +17,7 @@
 #include "clang/AST/Type.h"
 #include "clang/Basic/Diagnostic.h"
 #include "clang/Basic/IdentifierTable.h"
+#include "clang/Basic/OperatorKinds.h"
 #include "clang/Basic/PartialDiagnostic.h"
 #include "clang/Basic/SourceLocation.h"
 #include "llvm/ADT/DenseMapInfo.h"
@@ -32,65 +33,195 @@
 
 class ASTContext;
 template  class CanQual;
-class CXXDeductionGuideNameExtra;
-class CXXLiteralOperatorIdName;
-class CXXOperatorIdName;
-class CXXSpecialName;
-class DeclarationNameExtra;
-class IdentifierInfo;
+class DeclarationName;
+class DeclarationNameTable;
 class MultiKeywordSelector;
-enum OverloadedOperatorKind : int;
 struct PrintingPolicy;
 class TemplateDecl;
 class TypeSourceInfo;
 class UsingDirectiveDecl;
 
 using CanQualType = CanQual;
 
-/// DeclarationName - The name of a declaration. In the common case,
-/// this just stores an IdentifierInfo pointer to a normal
-/// name. However, it also provides encodings for Objective-C
-/// selectors (optimizing zero- and one-argument selectors, which make
-/// up 78% percent of all selectors in Cocoa.h) and special C++ names
-/// for constructors, destructors, and conversion functions.
-class DeclarationName {
+namespace detail {
+
+/// CXXSpecialNameExtra records the type associated with one of the "special"
+/// kinds of declaration names in C++, e.g., constructors, destructors, and
+/// conversion functions. Note that CXXSpecialName is used for C++ constructor,
+/// destructor and conversion functions, but the actual kind is not stored in
+/// CXXSpecialName. Instead we use three different FoldingSet
+/// in DeclarationNameTable.
+class alignas(IdentifierInfoAlignment) CXXSpecialNameExtra
+: public llvm::FoldingSetNode {
+  friend class clang::DeclarationName;
+  friend class clang::DeclarationNameTable;
+
+  /// The type associated with this declaration name.
+  QualType Type;
+
+  /// Extra information associated with this declaration name that
+  /// can be used by the front end. All bits are really needed
+  /// so it is not possible to stash something in the low order bits.
+  void *FETokenInfo;
+
+  CXXSpecialNameExtra(QualType QT) : Type(QT), FETokenInfo(nullptr) {}
+
 public:
-  /// NameKind - The kind of name this object contains.
-  enum NameKind {
-Identifier,
-ObjCZeroArgSelector,
-ObjCOneArgSelector,
-ObjCMultiArgSelector,
-CXXConstructorName,
-CXXDestructorName,
-CXXConversionFunctionName,
-CXXDeductionGuideName,
-CXXOperatorName,
-CXXLiteralOperatorName,
-CXXUsingDirective
-  };
+  void Profile(llvm::FoldingSetNodeID ) {
+ID.AddPointer(Type.getAsOpaquePtr());
+  }
+};
 
-  static const unsigned NumNameKinds = CXXUsingDirective + 1;
+/// Contains extra information for the name of a C++ deduction guide.
+class alignas(IdentifierInfoAlignment) CXXDeductionGuideNameExtra
+: public detail::DeclarationNameExtra,
+  public llvm::FoldingSetNode {
+  friend class clang::DeclarationName;
+  friend class clang::DeclarationNameTable;
 
-private:
+  /// The template named by the deduction guide.
+  TemplateDecl *Template;
+
+  /// Extra information associated with this operator name that
+  /// can be used by the front end. All bits are really needed
+  /// so it is not possible to stash something in the low order bits.
+  void *FETokenInfo;
+
+  CXXDeductionGuideNameExtra(TemplateDecl *TD)
+  : DeclarationNameExtra(CXXDeductionGuideName), Template(TD),
+FETokenInfo(nullptr) {}
+
+public:
+  void Profile(llvm::FoldingSetNodeID ) { ID.AddPointer(Template); }
+};
+
+/// Contains extra information for the name of an overloaded operator
+/// in C++, such as "operator+. This do not includes literal or conversion
+/// operators. For literal operators see CXXLiteralOperatorIdName and for
+/// conversion operators see CXXSpecialNameExtra.
+class alignas(IdentifierInfoAlignment) CXXOperatorIdName {
+  friend class clang::DeclarationName;
+  friend class clang::DeclarationNameTable;
+
+  /// The kind of this operator.
+  OverloadedOperatorKind Kind = OO_None;
+
+  /// Extra information associated with this operator name that
+  /// can be used by the front end. All bits are really needed
+  /// so it is not possible to stash something in the low order bits.
+  void *FETokenInfo 

[PATCH] D52267: [AST] Various optimizations + refactoring in DeclarationName(Table)

[John McCall via Phabricator via cfe-commits]

rjmccall accepted this revision.
rjmccall added inline comments.
This revision is now accepted and ready to land.



Comment at: include/clang/AST/DeclarationName.h:46
 
-/// DeclarationName - The name of a declaration. In the common case,
-/// this just stores an IdentifierInfo pointer to a normal
-/// name. However, it also provides encodings for Objective-C
-/// selectors (optimizing zero- and one-argument selectors, which make
-/// up 78% percent of all selectors in Cocoa.h) and special C++ names
-/// for constructors, destructors, and conversion functions.
+namespace detail {
+

riccibruno wrote:
> rjmccall wrote:
> > Should `DeclarationNameExtra` be moved here?  I'm not sure why it's in 
> > `IdentifierTable.h` in the first place, and your refactor seems to make 
> > that even more pointless.
> `DeclarationNameExtra` is unfortunately needed by `MultiKeywordSelector`
> in `IdentifierInfo.cpp`.
Oh, and one is in Basic instead of AST.

I'm not sure there's really a good reason for Selector to exist at the Basic 
level instead of AST; it's probably more an artifact of old divisions than 
something that's really useful.  But changing that would probably be painful.



Comment at: include/clang/AST/DeclarationName.h:164
+CXXUsingDirective = 10,
+ObjCMultiArgSelector = 11
+  };

riccibruno wrote:
> rjmccall wrote:
> > Is the criterion for inclusion in the first seven really just frequency of 
> > use, or is it a combination of that and relative benefit?
> > 
> > The only one I would really quibble about is that multi-argument selectors 
> > are probably more common and important to Objective-C code than conversion 
> > operators are to C++.  But it's quite possible that the relative benefit is 
> > much higher for C++, since selectors only appear on specific kinds of 
> > declarations that know they're declared with selectors — relatively little 
> > code actually needs to be polymorphic about them — and since they have to 
> > be defined out-of-line.
> I did not do an in-depth analysis regarding the selection of the first seven 
> inline kinds.
> My thought process was that C++ constructors and operator names should 
> definitely be
> inline. C++ destructors seemed much more frequent than c++ literal operators 
> and
> deduction guides. This leaves one slot available and since C++ conversion 
> operators
> share the same class `CXXSpecialName` including it as an inline kind 
> simplifies the code.
> 
> Also a practical point is that I don't really have a representative sample of 
> ObjC code
> to benchmark this. For C++ I am using all of Boost which I hope is 
> representative
> enough. If you deem it necessary I will try to do some benchmarks with
> `ObjCMultiArgSelector` stored inline.
I think I can accept this as-is without extra investigation.



Comment at: include/clang/AST/DeclarationName.h:220
+   detail::DeclarationNameExtra::ObjCMultiArgSelector
   };
 

Thanks, this looks great.


Repository:
  rC Clang

https://reviews.llvm.org/D52267



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

[PATCH] D52267: [AST] Various optimizations + refactoring in DeclarationName(Table)

[Bruno Ricci via Phabricator via cfe-commits]

riccibruno added a comment.

Addressed some inline comments.




Comment at: include/clang/AST/DeclarationName.h:46
 
-/// DeclarationName - The name of a declaration. In the common case,
-/// this just stores an IdentifierInfo pointer to a normal
-/// name. However, it also provides encodings for Objective-C
-/// selectors (optimizing zero- and one-argument selectors, which make
-/// up 78% percent of all selectors in Cocoa.h) and special C++ names
-/// for constructors, destructors, and conversion functions.
+namespace detail {
+

rjmccall wrote:
> Should `DeclarationNameExtra` be moved here?  I'm not sure why it's in 
> `IdentifierTable.h` in the first place, and your refactor seems to make that 
> even more pointless.
`DeclarationNameExtra` is unfortunately needed by `MultiKeywordSelector`
in `IdentifierInfo.cpp`.



Comment at: include/clang/AST/DeclarationName.h:164
+CXXUsingDirective = 10,
+ObjCMultiArgSelector = 11
+  };

rjmccall wrote:
> Is the criterion for inclusion in the first seven really just frequency of 
> use, or is it a combination of that and relative benefit?
> 
> The only one I would really quibble about is that multi-argument selectors 
> are probably more common and important to Objective-C code than conversion 
> operators are to C++.  But it's quite possible that the relative benefit is 
> much higher for C++, since selectors only appear on specific kinds of 
> declarations that know they're declared with selectors — relatively little 
> code actually needs to be polymorphic about them — and since they have to be 
> defined out-of-line.
I did not do an in-depth analysis regarding the selection of the first seven 
inline kinds.
My thought process was that C++ constructors and operator names should 
definitely be
inline. C++ destructors seemed much more frequent than c++ literal operators and
deduction guides. This leaves one slot available and since C++ conversion 
operators
share the same class `CXXSpecialName` including it as an inline kind simplifies 
the code.

Also a practical point is that I don't really have a representative sample of 
ObjC code
to benchmark this. For C++ I am using all of Boost which I hope is 
representative
enough. If you deem it necessary I will try to do some benchmarks with
`ObjCMultiArgSelector` stored inline.


Repository:
  rC Clang

https://reviews.llvm.org/D52267



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

[PATCH] D52267: [AST] Various optimizations + refactoring in DeclarationName(Table)

[Bruno Ricci via Phabricator via cfe-commits]

riccibruno updated this revision to Diff 166324.
riccibruno marked 9 inline comments as done.
riccibruno added a comment.

Address rjmccall comments:

1. Renamed `CXXSpecialName` to `CXXSpecialNameExtra`
2. Introduced a constant `IdentifierInfoAlignment` for 
`alignas(IdentifierInfoAlignment)`
3. Updated some comments
4. Cleaned up the `static_assert` which checked the consistency of the 
numerical values of the enumerators.

Regarding using a `PointerIntPair` it would work if we pass a custom
`PointerLikeTypeTraits` to state that, yes, our `void *` are aligned to 8 bytes
instead of 4. However I am not sure this is a good idea since the 
`PointerIntPair`
do not really simplify the code. What we really want here is a pointer union 
but we
also need precise control over the low bits.


Repository:
  rC Clang

https://reviews.llvm.org/D52267

Files:
  include/clang/AST/DeclarationName.h
  include/clang/Basic/IdentifierTable.h
  lib/AST/DeclarationName.cpp
  lib/Basic/IdentifierTable.cpp
  lib/Sema/IdentifierResolver.cpp
  lib/Serialization/ASTReader.cpp
  lib/Serialization/ASTWriter.cpp

Index: lib/Serialization/ASTWriter.cpp
===
--- lib/Serialization/ASTWriter.cpp
+++ lib/Serialization/ASTWriter.cpp
@@ -3574,7 +3574,7 @@
 II->isPoisoned() ||
 (IsModule ? II->hasRevertedBuiltin() : II->getObjCOrBuiltinID()) ||
 II->hasRevertedTokenIDToIdentifier() ||
-(NeedDecls && II->getFETokenInfo()))
+(NeedDecls && II->getFETokenInfo()))
   return true;
 
 return false;
Index: lib/Serialization/ASTReader.cpp
===
--- lib/Serialization/ASTReader.cpp
+++ lib/Serialization/ASTReader.cpp
@@ -908,7 +908,7 @@
  (IsModule ? II.hasRevertedBuiltin() : II.getObjCOrBuiltinID()) ||
  II.hasRevertedTokenIDToIdentifier() ||
  (!(IsModule && Reader.getPreprocessor().getLangOpts().CPlusPlus) &&
-  II.getFETokenInfo());
+  II.getFETokenInfo());
 }
 
 static bool readBit(unsigned ) {
Index: lib/Sema/IdentifierResolver.cpp
===
--- lib/Sema/IdentifierResolver.cpp
+++ lib/Sema/IdentifierResolver.cpp
@@ -147,7 +147,7 @@
   if (IdentifierInfo *II = Name.getAsIdentifierInfo())
 updatingIdentifier(*II);
 
-  void *Ptr = Name.getFETokenInfo();
+  void *Ptr = Name.getFETokenInfo();
 
   if (!Ptr) {
 Name.setFETokenInfo(D);
@@ -172,7 +172,7 @@
   if (IdentifierInfo *II = Name.getAsIdentifierInfo())
 updatingIdentifier(*II);
 
-  void *Ptr = Name.getFETokenInfo();
+  void *Ptr = Name.getFETokenInfo();
 
   if (!Ptr) {
 AddDecl(D);
@@ -213,7 +213,7 @@
   if (IdentifierInfo *II = Name.getAsIdentifierInfo())
 updatingIdentifier(*II);
 
-  void *Ptr = Name.getFETokenInfo();
+  void *Ptr = Name.getFETokenInfo();
 
   assert(Ptr && "Didn't find this decl on its identifier's chain!");
 
@@ -232,7 +232,7 @@
   if (IdentifierInfo *II = Name.getAsIdentifierInfo())
 readingIdentifier(*II);
 
-  void *Ptr = Name.getFETokenInfo();
+  void *Ptr = Name.getFETokenInfo();
   if (!Ptr) return end();
 
   if (isDeclPtr(Ptr))
@@ -304,7 +304,7 @@
   if (IdentifierInfo *II = Name.getAsIdentifierInfo())
 readingIdentifier(*II);
 
-  void *Ptr = Name.getFETokenInfo();
+  void *Ptr = Name.getFETokenInfo();
 
   if (!Ptr) {
 Name.setFETokenInfo(D);
@@ -397,7 +397,7 @@
 /// It creates a new IdDeclInfo if one was not created before for this id.
 IdentifierResolver::IdDeclInfo &
 IdentifierResolver::IdDeclInfoMap::operator[](DeclarationName Name) {
-  void *Ptr = Name.getFETokenInfo();
+  void *Ptr = Name.getFETokenInfo();
 
   if (Ptr) return *toIdDeclInfo(Ptr);
 
@@ -415,7 +415,7 @@
 
 void IdentifierResolver::iterator::incrementSlowCase() {
   NamedDecl *D = **this;
-  void *InfoPtr = D->getDeclName().getFETokenInfo();
+  void *InfoPtr = D->getDeclName().getFETokenInfo();
   assert(!isDeclPtr(InfoPtr) && "Decl with wrong id ?");
   IdDeclInfo *Info = toIdDeclInfo(InfoPtr);
 
Index: lib/Basic/IdentifierTable.cpp
===
--- lib/Basic/IdentifierTable.cpp
+++ lib/Basic/IdentifierTable.cpp
@@ -382,50 +382,49 @@
 
 namespace clang {
 
-/// MultiKeywordSelector - One of these variable length records is kept for each
+/// One of these variable length records is kept for each
 /// selector containing more than one keyword. We use a folding set
 /// to unique aggregate names (keyword selectors in ObjC parlance). Access to
 /// this class is provided strictly through Selector.
-class MultiKeywordSelector
-  : public DeclarationNameExtra, public llvm::FoldingSetNode {
-  MultiKeywordSelector(unsigned nKeys) {
-ExtraKindOrNumArgs = NUM_EXTRA_KINDS + nKeys;
-  }
+class alignas(IdentifierInfoAlignment) MultiKeywordSelector
+: public detail::DeclarationNameExtra,
+  public llvm::FoldingSetNode {
+  

[PATCH] D52267: [AST] Various optimizations + refactoring in DeclarationName(Table)

[John McCall via Phabricator via cfe-commits]

rjmccall added a comment.

Conceptually, this change looks great.  And it should be fine to require extra 
alignment on `IdentifierInfo` on 32-bit hosts; I doubt that will have 
measurable impact.

I believe it's possible to eliminate the need for most, perhaps all, of these 
`static_asserts` by just defining the constants more sensibly in the first 
place.




Comment at: include/clang/AST/DeclarationName.h:46
 
-/// DeclarationName - The name of a declaration. In the common case,
-/// this just stores an IdentifierInfo pointer to a normal
-/// name. However, it also provides encodings for Objective-C
-/// selectors (optimizing zero- and one-argument selectors, which make
-/// up 78% percent of all selectors in Cocoa.h) and special C++ names
-/// for constructors, destructors, and conversion functions.
+namespace detail {
+

Should `DeclarationNameExtra` be moved here?  I'm not sure why it's in 
`IdentifierTable.h` in the first place, and your refactor seems to make that 
even more pointless.



Comment at: include/clang/AST/DeclarationName.h:54
+/// we use three different FoldingSet in DeclarationNameTable.
+class alignas(8) CXXSpecialName : public llvm::FoldingSetNode {
+  friend class clang::DeclarationName;

If this isn't a self-contained description of the name anymore, I think adding 
`Extra` to the class name would be appropriate.  And maybe the class name 
should directly express that is for the kinds of names that are basically 
uniqued by type.

Please introduce a symbolic constant for 8 here, since you use and assume it 
across multiple places.  Either `#define` or a `const` variable works.



Comment at: include/clang/AST/DeclarationName.h:98
+/// Contains extra information for the name of an overloaded
+/// operator in C++, such as "operator+.
+class alignas(8) CXXOperatorIdName {

This is probably pre-existing, but the comment should clarify that this doesn't 
include literal or conversion operators.



Comment at: include/clang/AST/DeclarationName.h:164
+CXXUsingDirective = 10,
+ObjCMultiArgSelector = 11
+  };

Is the criterion for inclusion in the first seven really just frequency of use, 
or is it a combination of that and relative benefit?

The only one I would really quibble about is that multi-argument selectors are 
probably more common and important to Objective-C code than conversion 
operators are to C++.  But it's quite possible that the relative benefit is 
much higher for C++, since selectors only appear on specific kinds of 
declarations that know they're declared with selectors — relatively little code 
actually needs to be polymorphic about them — and since they have to be defined 
out-of-line.



Comment at: include/clang/AST/DeclarationName.h:178
+  ///   This enable efficient conversion between the two enumerations
+  ///   in the usual case.
+  ///

You can express this directly by defining `StoredNameKind` first and then 
defining the corresponding enumerators in `NameKind` in terms of them.



Comment at: include/clang/AST/DeclarationName.h:184
+  ///   between DeclarationNameExtra::ExtraKind and NameKind possible with
+  ///   a single addition/substraction.
+  ///

Same deal.  Just define the enumerators of `NameKind` appropriately.



Comment at: include/clang/AST/DeclarationName.h:243
+  ///   C++ literal operator, or C++ using directive.
   uintptr_t Ptr = 0;
 

riccibruno wrote:
> erichkeane wrote:
> > There is an llvm type for storing something in the lower bits of a pointer. 
> >  I THINK it is llvm::PointerIntPair.
> > 
> > I'd MUCH prefer you do that instead of the reimplementation here.
> Well it was already like this but point taken.
Using `PointerIntPair` probably won't work here because the pointer type is 
basically a union, and you can't use `void*` because `PointerIntPair` wants to 
assert that the pointer type is sufficiently aligned.


Repository:
  rC Clang

https://reviews.llvm.org/D52267



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

[PATCH] D52267: [AST] Various optimizations + refactoring in DeclarationName(Table)

[Bruno Ricci via Phabricator via cfe-commits]

riccibruno added inline comments.



Comment at: include/clang/AST/DeclarationName.h:243
+  ///   C++ literal operator, or C++ using directive.
   uintptr_t Ptr = 0;
 

erichkeane wrote:
> There is an llvm type for storing something in the lower bits of a pointer.  
> I THINK it is llvm::PointerIntPair.
> 
> I'd MUCH prefer you do that instead of the reimplementation here.
Well it was already like this but point taken.



Comment at: lib/AST/DeclarationName.cpp:42
 
+void DeclarationName::VerifyStaticAsserts() {
+  llvm_unreachable("VerifyStaticAsserts is not meant to be called!");

erichkeane wrote:
> I'm a touch confused by this function.  It shouldn't be necessary when the 
> static asserts can all be put at the global/class level.
Yes, but the enumeration StoredNameKind is private to DeclarationName.
Therefore I cannot just put the static_asserts into a .cpp and be done with it.
An alternative is of course to put them in the class definition in the header 
but
look at this mess... (and this is after clang-format).


Repository:
  rC Clang

https://reviews.llvm.org/D52267



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

[PATCH] D52267: [AST] Various optimizations + refactoring in DeclarationName(Table)

[Erich Keane via Phabricator via cfe-commits]

erichkeane added a comment.

A few drive-by comments... This is a patch with interactions I'm not sure I 
feel comfortable approving myself, so I'll count on @rjmccall to do so.




Comment at: include/clang/AST/DeclarationName.h:243
+  ///   C++ literal operator, or C++ using directive.
   uintptr_t Ptr = 0;
 

There is an llvm type for storing something in the lower bits of a pointer.  I 
THINK it is llvm::PointerIntPair.

I'd MUCH prefer you do that instead of the reimplementation here.



Comment at: lib/AST/DeclarationName.cpp:42
 
+void DeclarationName::VerifyStaticAsserts() {
+  llvm_unreachable("VerifyStaticAsserts is not meant to be called!");

I'm a touch confused by this function.  It shouldn't be necessary when the 
static asserts can all be put at the global/class level.


Repository:
  rC Clang

https://reviews.llvm.org/D52267



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

[PATCH] D52267: [AST] Various optimizations + refactoring in DeclarationName(Table)

[Bruno Ricci via Phabricator via cfe-commits]

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

Introduce the following optimizations in DeclarationName(Table)

1. Store common kinds inline in DeclarationName instead of 
DeclarationNameExtra. Currently the kind of C++ constructor, destructor, 
conversion function and overloaded operator names is stored in 
DeclarationNameExtra. Instead store it inline in DeclarationName. To do this 
align IdentifierInfo, CXXSpecialName, DeclarationNameExtra and 
CXXOperatorIdName to 8 bytes so that we can use the lower 3 bits of 
DeclarationName::Ptr. This is already the case on 64 bits archs anyway. This 
also allow us to remove DeclarationNameExtra from CXXSpecialName and 
CXXOperatorIdName, which shave off a pointer from CXXSpecialName. (This might 
be the thing that sink this patch since you might judge that aligning 
IdentifierInfo to 8 bytes even on 32 bits archs is not acceptable. Although if 
we care about 32 bits archs there is probably a lot more that can be done.)

2. Synchronize the enumerations DeclarationName::NameKind, 
DeclarationName::StoredNameKind and Selector::IdentifierInfoFlag. This makes 
DeclarationName::getNameKind much more efficient since we can replace the 
switch table by a single comparison and an addition. static_asserts are 
provided in DeclarationName::VerifyStaticAsserts to ensure that the 
enumerations remain in sync.

3. Put the overloaded operator names inline in DeclarationNameTable to remove 
an indirection. This increase the size of DeclarationNameTable a little bit but 
this is not important since it is only used in ASTContext, and never copied nor 
moved from. This also get rid of the last dynamic allocation in 
DeclarationNameTable.

Altogether these optimizations cut the run time of parsing all of Boost by 
about 0.8%.
While we are at it, do the following NFC modifications:

1. Put the internal classes CXXSpecialName, CXXDeductionGuideNameExtra, 
CXXOperatorIdName, CXXLiteralOperatorIdName and DeclarationNameExtra in a 
namespace detail since these classes are only meant to be used by 
DeclarationName and DeclarationNameTable. Make this more explicit by making the 
members of these classes private and friending DeclarationName(Table).

2. Make DeclarationName::getFETokenInfo a non-template since every users are 
using it to get a void *. It was supposed to be used with a type to avoid a 
subsequent static_cast.

3. Change the internal functions DeclarationName::getAs* to castAs* since when 
we use them we already know the correct kind. This has no external impact since 
all of these are private.


Repository:
  rC Clang

https://reviews.llvm.org/D52267

Files:
  include/clang/AST/DeclarationName.h
  include/clang/Basic/IdentifierTable.h
  lib/AST/DeclarationName.cpp
  lib/Basic/IdentifierTable.cpp
  lib/Sema/IdentifierResolver.cpp
  lib/Serialization/ASTReader.cpp
  lib/Serialization/ASTWriter.cpp

Index: lib/Serialization/ASTWriter.cpp
===
--- lib/Serialization/ASTWriter.cpp
+++ lib/Serialization/ASTWriter.cpp
@@ -3574,7 +3574,7 @@
 II->isPoisoned() ||
 (IsModule ? II->hasRevertedBuiltin() : II->getObjCOrBuiltinID()) ||
 II->hasRevertedTokenIDToIdentifier() ||
-(NeedDecls && II->getFETokenInfo()))
+(NeedDecls && II->getFETokenInfo()))
   return true;
 
 return false;
Index: lib/Serialization/ASTReader.cpp
===
--- lib/Serialization/ASTReader.cpp
+++ lib/Serialization/ASTReader.cpp
@@ -908,7 +908,7 @@
  (IsModule ? II.hasRevertedBuiltin() : II.getObjCOrBuiltinID()) ||
  II.hasRevertedTokenIDToIdentifier() ||
  (!(IsModule && Reader.getPreprocessor().getLangOpts().CPlusPlus) &&
-  II.getFETokenInfo());
+  II.getFETokenInfo());
 }
 
 static bool readBit(unsigned ) {
Index: lib/Sema/IdentifierResolver.cpp
===
--- lib/Sema/IdentifierResolver.cpp
+++ lib/Sema/IdentifierResolver.cpp
@@ -147,7 +147,7 @@
   if (IdentifierInfo *II = Name.getAsIdentifierInfo())
 updatingIdentifier(*II);
 
-  void *Ptr = Name.getFETokenInfo();
+  void *Ptr = Name.getFETokenInfo();
 
   if (!Ptr) {
 Name.setFETokenInfo(D);
@@ -172,7 +172,7 @@
   if (IdentifierInfo *II = Name.getAsIdentifierInfo())
 updatingIdentifier(*II);
 
-  void *Ptr = Name.getFETokenInfo();
+  void *Ptr = Name.getFETokenInfo();
 
   if (!Ptr) {
 AddDecl(D);
@@ -213,7 +213,7 @@
   if (IdentifierInfo *II = Name.getAsIdentifierInfo())
 updatingIdentifier(*II);
 
-  void *Ptr = Name.getFETokenInfo();
+  void *Ptr = Name.getFETokenInfo();
 
   assert(Ptr && "Didn't find this decl on its identifier's chain!");
 
@@ -232,7 +232,7 @@
   if (IdentifierInfo *II = Name.getAsIdentifierInfo())