Store intrinsic ID by value in Function instead of a string lookup. NFC

[pete]

Summary

On 64-bit targets, Function has 4-bytes of padding in its struct layout.

This uses the space for the intrinsic ID. It is set and recalculated whenever the function name is set. This is similar to the current behavior which clears the function from the intrinsic ID cache when its renamed.

The intrinsic cache itself is removed as the only purpose was to speedup calls to getIntrinsicID() which now just reading the new field in the struct.

[rnk]

You are a hero for doing this. :) I was looking at this the other day and thinking "really, a DenseMap to look up a 32-bit integer?"

[pete]

Duncan gave a LGTM by email. Committed as r237641 and r237642.

[pete]

Forgot to actually close this long since committed code.

include/llvm/IR/Function.h

void CheckLazyArguments() const {
     if (hasLazyArguments())
       BuildLazyArguments();
   }
   void BuildLazyArguments() const;
 
   Function(const Function&) = delete;
   void operator=(const Function&) = delete;
 
-  /// Do the actual lookup of an intrinsic ID when the query could not be
-  /// answered from the cache.
-  unsigned lookupIntrinsicID() const LLVM_READONLY;
-
   /// Function ctor - If the (optional) Module argument is specified, the
   /// function is automatically inserted into the end of the function list for
   /// the module.
   ///
   Function(FunctionType *Ty, LinkageTypes Linkage,
            const Twine &N = "", Module *M = nullptr);
 
 public:
public:
   bool isMaterializable() const;
   void setIsMaterializable(bool V);
 
   /// getIntrinsicID - This method returns the ID number of the specified
   /// function, or Intrinsic::not_intrinsic if the function is not an
   /// intrinsic, or if the pointer is null.  This value is always defined to be
   /// zero to allow easy checking for whether a function is intrinsic or not.
   /// The particular intrinsic functions which correspond to this value are
-  /// defined in llvm/Intrinsics.h.  Results are cached in the LLVM context,
-  /// subsequent requests for the same ID return results much faster from the
-  /// cache.
+  /// defined in llvm/Intrinsics.h.
   ///
-  unsigned getIntrinsicID() const LLVM_READONLY;
+  unsigned getIntrinsicID() const LLVM_READONLY { return IntID; }
   bool isIntrinsic() const { return getName().startswith("llvm."); }
 
+  /// \brief Recalculate the ID for this function if it is an Intrinsic defined
+  /// in llvm/Intrinsics.h.  Sets the intrinsic ID to Intrinsic::not_intrinsic
+  /// if the name of this function does not match an intrinsic in that header.
+  void recalculateIntrinsicID();
+
   /// getCallingConv()/setCallingConv(CC) - These method get and set the
   /// calling convention of this function.  The enum values for the known
   /// calling conventions are defined in CallingConv.h.
   CallingConv::ID getCallingConv() const {
     return static_cast<CallingConv::ID>(getSubclassDataFromValue() >> 3);
   }
   void setCallingConv(CallingConv::ID CC) {
     setValueSubclassData((getSubclassDataFromValue() & 7) |

include/llvm/IR/GlobalValue.h

 #include <system_error>
 
 namespace llvm {
 
 class Comdat;
 class PointerType;
 class Module;
 
+namespace Intrinsic {
+  enum ID : unsigned;
+};
+
 class GlobalValue : public Constant {
   GlobalValue(const GlobalValue &) = delete;
 public:
   /// @brief An enumeration for the kinds of linkage for global values.
   enum LinkageTypes {
     ExternalLinkage = 0,///< Externally visible function
     AvailableExternallyLinkage, ///< Available for inspection, not emission.
     LinkOnceAnyLinkage, ///< Keep one copy of function when linking (inline)
public:
   };
 
 protected:
   GlobalValue(PointerType *Ty, ValueTy VTy, Use *Ops, unsigned NumOps,
               LinkageTypes Linkage, const Twine &Name)
       : Constant(Ty, VTy, Ops, NumOps), Linkage(Linkage),
         Visibility(DefaultVisibility), UnnamedAddr(0),
         DllStorageClass(DefaultStorageClass),
-        ThreadLocal(NotThreadLocal), Parent(nullptr) {
+        ThreadLocal(NotThreadLocal), IntID((Intrinsic::ID)0U), Parent(nullptr) {
     setName(Name);
   }
 
   // Note: VC++ treats enums as signed, so an extra bit is required to prevent
   // Linkage and Visibility from turning into negative values.
   LinkageTypes Linkage : 5;   // The linkage of this global
   unsigned Visibility : 2;    // The visibility style of this global
   unsigned UnnamedAddr : 1;   // This value's address is not significant
   unsigned DllStorageClass : 2; // DLL storage class
 
   unsigned ThreadLocal : 3; // Is this symbol "Thread Local", if so, what is
                             // the desired model?
 
 private:
   // Give subclasses access to what otherwise would be wasted padding.
   // (19 + 3 + 2 + 1 + 2 + 5) == 32.
   unsigned SubClassData : 19;
+
 protected:
+  /// \brief The intrinsic ID for this subclass (which must be a Function).
+  ///
+  /// This member is defined by this class, but not used for anything.
+  /// Subclasses can use it to store their intrinsic ID, if they have one.
+  ///
+  /// This is stored here to save space in Function on 64-bit hosts.
+  Intrinsic::ID IntID;
+
   static const unsigned GlobalValueSubClassDataBits = 19;
   unsigned getGlobalValueSubClassData() const {
     return SubClassData;
   }
   void setGlobalValueSubClassData(unsigned V) {
     assert(V < (1 << 19) && "It will not fit");
     SubClassData = V;
   }

include/llvm/IR/Intrinsics.h

 class LLVMContext;
 class Module;
 class AttributeSet;
 
 /// This namespace contains an enum with a value for every intrinsic/builtin
 /// function known by LLVM. The enum values are returned by
 /// Function::getIntrinsicID().
 namespace Intrinsic {
-  enum ID {
+  enum ID : unsigned {
     not_intrinsic = 0,   // Must be zero
 
     // Get the intrinsic enums generated from Intrinsics.td
 #define GET_INTRINSIC_ENUM_VALUES
 #include "llvm/IR/Intrinsics.gen"
 #undef GET_INTRINSIC_ENUM_VALUES
     , num_intrinsics
   };

include/llvm/IR/Value.h

public:
 
   // \brief All values can potentially be named.
   bool hasName() const { return getValueName() != nullptr; }
   ValueName *getValueName() const { return NameAndIsUsedByMD.getPointer(); }
   void setValueName(ValueName *VN) { NameAndIsUsedByMD.setPointer(VN); }
 
 private:
   void destroyValueName();
+  void setNameImpl(const Twine &Name);
 
 public:
   /// \brief Return a constant reference to the value's name.
   ///
   /// This is cheap and guaranteed to return the same reference as long as the
   /// value is not modified.
   StringRef getName() const;
 

lib/IR/Function.cpp

Function::Function(FunctionType *Ty, LinkageTypes Linkage, const Twine &name,
   // If the function has arguments, mark them as lazily built.
   if (Ty->getNumParams())
     setValueSubclassData(1);   // Set the "has lazy arguments" bit.
 
   if (ParentModule)
     ParentModule->getFunctionList().push_back(this);
 
   // Ensure intrinsics have the right parameter attributes.
-  if (unsigned IID = getIntrinsicID())
-    setAttributes(Intrinsic::getAttributes(getContext(), Intrinsic::ID(IID)));
+  // Note, the IntID field will have been set in Value::setName if this function
+  // name is a valid intrinsic ID.
+  if (IntID)
+    setAttributes(Intrinsic::getAttributes(getContext(), IntID));
 
 }
 
 Function::~Function() {
   dropAllReferences();    // After this it is safe to delete instructions.
 
   // Delete all of the method arguments and unlink from symbol table...
   ArgumentList.clear();
   delete SymTab;
 
   // Remove the function from the on-the-side GC table.
   clearGC();
-
-  // Remove the intrinsicID from the Cache.
-  if (getValueName() && isIntrinsic())
-    getContext().pImpl->IntrinsicIDCache.erase(this);
 }
 
 void Function::BuildLazyArguments() const {
   // Create the arguments vector, all arguments start out unnamed.
   FunctionType *FT = getFunctionType();
   for (unsigned i = 0, e = FT->getNumParams(); i != e; ++i) {
     assert(!FT->getParamType(i)->isVoidTy() &&
            "Cannot have void typed arguments!");
void Function::copyAttributesFrom(const GlobalValue *Src) {
   else
     setPrefixData(nullptr);
   if (SrcF->hasPrologueData())
     setPrologueData(SrcF->getPrologueData());
   else
     setPrologueData(nullptr);
 }
 
-/// getIntrinsicID - This method returns the ID number of the specified
-/// function, or Intrinsic::not_intrinsic if the function is not an
-/// intrinsic, or if the pointer is null.  This value is always defined to be
-/// zero to allow easy checking for whether a function is intrinsic or not.  The
-/// particular intrinsic functions which correspond to this value are defined in
-/// llvm/Intrinsics.h.  Results are cached in the LLVM context, subsequent
-/// requests for the same ID return results much faster from the cache.
-///
-unsigned Function::getIntrinsicID() const {
-  const ValueName *ValName = this->getValueName();
-  if (!ValName || !isIntrinsic())
-    return 0;
-
-  LLVMContextImpl::IntrinsicIDCacheTy &IntrinsicIDCache =
-    getContext().pImpl->IntrinsicIDCache;
-  if (!IntrinsicIDCache.count(this)) {
-    unsigned Id = lookupIntrinsicID();
-    IntrinsicIDCache[this]=Id;
-    return Id;
-  }
-  return IntrinsicIDCache[this];
-}
-
-/// This private method does the actual lookup of an intrinsic ID when the query
-/// could not be answered from the cache.
-unsigned Function::lookupIntrinsicID() const {
-  const ValueName *ValName = this->getValueName();
+/// \brief This does the actual lookup of an intrinsic ID which
+/// matches the given function name.
+static Intrinsic::ID lookupIntrinsicID(const ValueName *ValName) {
   unsigned Len = ValName->getKeyLength();
   const char *Name = ValName->getKeyData();
 
 #define GET_FUNCTION_RECOGNIZER
 #include "llvm/IR/Intrinsics.gen"
 #undef GET_FUNCTION_RECOGNIZER
 
-  return 0;
+  return Intrinsic::not_intrinsic;
+}
+
+void Function::recalculateIntrinsicID() {
+  const ValueName *ValName = this->getValueName();
+  if (!ValName || !isIntrinsic()) {
+    IntID = Intrinsic::not_intrinsic;
+    return;
+  }
+  IntID = lookupIntrinsicID(ValName);
 }
 
 /// Returns a stable mangling for the type specified for use in the name
 /// mangling scheme used by 'any' types in intrinsic signatures.  The mangling
 /// of named types is simply their name.  Manglings for unnamed types consist
 /// of a prefix ('p' for pointers, 'a' for arrays, 'f_' for functions)
 /// combined with the mangling of their component types.  A vararg function
 /// type will have a suffix of 'vararg'.  Since function types can contain

lib/IR/LLVMContextImpl.h

#include "llvm/IR/Metadata.def"
   /// Collection of per-function metadata used in this context.
   DenseMap<const Function *, MDAttachmentMap> FunctionMetadata;
 
   /// DiscriminatorTable - This table maps file:line locations to an
   /// integer representing the next DWARF path discriminator to assign to
   /// instructions in different blocks at the same location.
   DenseMap<std::pair<const char *, unsigned>, unsigned> DiscriminatorTable;
 
-  /// IntrinsicIDCache - Cache of intrinsic name (string) to numeric ID mappings
-  /// requested in this context
-  typedef DenseMap<const Function*, unsigned> IntrinsicIDCacheTy;
-  IntrinsicIDCacheTy IntrinsicIDCache;
-
   /// \brief Mapping from a function to its prefix data, which is stored as the
   /// operand of an unparented ReturnInst so that the prefix data has a Use.
   typedef DenseMap<const Function *, ReturnInst *> PrefixDataMapTy;
   PrefixDataMapTy PrefixDataMap;
 
   /// \brief Mapping from a function to its prologue data, which is stored as
   /// the operand of an unparented ReturnInst so that the prologue data has a
   /// Use.

lib/IR/Value.cpp

StringRef Value::getName() const {
   // Make sure the empty string is still a C string. For historical reasons,
   // some clients want to call .data() on the result and expect it to be null
   // terminated.
   if (!getValueName())
     return StringRef("", 0);
   return getValueName()->getKey();
 }
 
-void Value::setName(const Twine &NewName) {
+void Value::setNameImpl(const Twine &NewName) {
   // Fast path for common IRBuilder case of setName("") when there is no name.
   if (NewName.isTriviallyEmpty() && !hasName())
     return;
 
   SmallString<256> NameData;
   StringRef NameRef = NewName.toStringRef(NameData);
   assert(NameRef.find_first_of(0) == StringRef::npos &&
          "Null bytes are not allowed in names");
 
   // Name isn't changing?
   if (getName() == NameRef)
     return;
 
   assert(!getType()->isVoidTy() && "Cannot assign a name to void values!");
 
   // Get the symbol table to update for this object.
   ValueSymbolTable *ST;
   if (getSymTab(this, ST))
     return;  // Cannot set a name on this value (e.g. constant).
 
-  if (Function *F = dyn_cast<Function>(this))
-    getContext().pImpl->IntrinsicIDCache.erase(F);
-
   if (!ST) { // No symbol table to update?  Just do the change.
     if (NameRef.empty()) {
       // Free the name for this value.
       destroyValueName();
       return;
     }
 
     // NOTE: Could optimize for the case the name is shrinking to not deallocate
if (hasName()) {
     if (NameRef.empty())
       return;
   }
 
   // Name is changing to something new.
   setValueName(ST->createValueName(NameRef, this));
 }
 
+void Value::setName(const Twine &NewName) {
+  setNameImpl(NewName);
+  if (Function *F = dyn_cast<Function>(this))
+    F->recalculateIntrinsicID();
+}
+
 void Value::takeName(Value *V) {
   ValueSymbolTable *ST = nullptr;
   // If this value has a name, drop it.
   if (hasName()) {
     // Get the symtab this is in.
     if (getSymTab(this, ST)) {
       // We can't set a name on this value, but we need to clear V's name if
       // it has one.