[PATCH] D22949: Speed up Function::isIntrinsic() by adding a bit to GlobalValue. NFC

[Justin Bogner via llvm-commits]

Justin Lebar <jlebar at google.com> writes:
> jlebar created this revision.
> jlebar added reviewers: bogner, arsenm, joker-eph.
> jlebar added subscribers: llvm-commits, sanjoy.
>
> Previously isIntrinsic() called getName().  This involves a hashtable
> lookup, so is nontrivially expensive.  And isIntrinsic() is called
> frequently, particularly by dyn_cast<IntrinsicInstr>.
>
> This patch steals a bit of IntID and uses that to store whether or not
> getName() starts with "llvm."

I'm a bit less comfortable with this change than I was with the other
one - it's more of a straight performance hack this way, whereas the
other way fit better in a future where target-specific intrinsics are
more of a first class idea.

I'll let others comment on whether or not this is worth it as a
hopefully temporary thing for the performance gain in the short term.

> https://reviews.llvm.org/D22949
>
> Files:
>   include/llvm/IR/Function.h
>   include/llvm/IR/GlobalValue.h
>   lib/IR/Function.cpp
>
> Index: lib/IR/Function.cpp
> ===================================================================
> --- lib/IR/Function.cpp
> +++ lib/IR/Function.cpp
> @@ -271,6 +271,7 @@
>    if (ParentModule)
>      ParentModule->getFunctionList().push_back(this);
>  
> +  HasLLVMReservedName = getName().startswith("llvm.");
>    // Ensure intrinsics have the right parameter attributes.
>    // Note, the IntID field will have been set in Value::setName if this function
>    // name is a valid intrinsic ID.
> @@ -488,9 +489,7 @@
>  
>  /// \brief This does the actual lookup of an intrinsic ID which
>  /// matches the given function name.
> -static Intrinsic::ID lookupIntrinsicID(const ValueName *ValName) {
> -  StringRef Name = ValName->getKey();
> -
> +static Intrinsic::ID lookupIntrinsicID(StringRef Name) {
>    ArrayRef<const char *> NameTable = findTargetSubtable(Name);
>    int Idx = Intrinsic::lookupLLVMIntrinsicByName(NameTable, Name);
>    if (Idx == -1)
> @@ -508,12 +507,14 @@
>  }
>  
>  void Function::recalculateIntrinsicID() {
> -  const ValueName *ValName = this->getValueName();
> -  if (!ValName || !isIntrinsic()) {
> +  StringRef Name = getName();
> +  if (!Name.startswith("llvm.")) {
> +    HasLLVMReservedName = false;
>      IntID = Intrinsic::not_intrinsic;
>      return;
>    }
> -  IntID = lookupIntrinsicID(ValName);
> +  HasLLVMReservedName = true;
> +  IntID = lookupIntrinsicID(Name);
>  }
>  
>  /// Returns a stable mangling for the type specified for use in the name
> Index: include/llvm/IR/GlobalValue.h
> ===================================================================
> --- include/llvm/IR/GlobalValue.h
> +++ include/llvm/IR/GlobalValue.h
> @@ -72,7 +72,7 @@
>          ValueType(Ty), Linkage(Linkage), Visibility(DefaultVisibility),
>          UnnamedAddrVal(unsigned(UnnamedAddr::None)),
>          DllStorageClass(DefaultStorageClass), ThreadLocal(NotThreadLocal),
> -        IntID((Intrinsic::ID)0U), Parent(nullptr) {
> +        IntID((Intrinsic::ID)0U), HasLLVMReservedName(false), Parent(nullptr) {
>      setName(Name);
>    }
>  
> @@ -128,7 +128,12 @@
>    /// 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;
> +  Intrinsic::ID IntID : 31;
> +
> +  /// True if the function's name starts with "llvm.".  This corresponds to the
> +  /// value of Function::isIntrinsic(), which may be true even if
> +  /// Function::intrinsicID() returns Intrinsic::not_intrinsic.
> +  bool HasLLVMReservedName : 1;
>  
>    unsigned getGlobalValueSubClassData() const {
>      return SubClassData;
> Index: include/llvm/IR/Function.h
> ===================================================================
> --- include/llvm/IR/Function.h
> +++ include/llvm/IR/Function.h
> @@ -137,7 +137,11 @@
>    /// The particular intrinsic functions which correspond to this value are
>    /// defined in llvm/Intrinsics.h.
>    Intrinsic::ID getIntrinsicID() const LLVM_READONLY { return IntID; }
> -  bool isIntrinsic() const { return getName().startswith("llvm."); }
> +
> +  /// isIntrinsic - Returns true if the function's name starts with "llvm.".
> +  /// It's possible for this function to return true while getIntrinsicID()
> +  /// returns Intrinsic::not_intrinsic!
> +  bool isIntrinsic() const { return HasLLVMReservedName; }
>  
>    /// \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
>
>
> Index: lib/IR/Function.cpp
> ===================================================================
> --- lib/IR/Function.cpp
> +++ lib/IR/Function.cpp
> @@ -271,6 +271,7 @@
>    if (ParentModule)
>      ParentModule->getFunctionList().push_back(this);
>  
> +  HasLLVMReservedName = getName().startswith("llvm.");
>    // Ensure intrinsics have the right parameter attributes.
>    // Note, the IntID field will have been set in Value::setName if this function
>    // name is a valid intrinsic ID.
> @@ -488,9 +489,7 @@
>  
>  /// \brief This does the actual lookup of an intrinsic ID which
>  /// matches the given function name.
> -static Intrinsic::ID lookupIntrinsicID(const ValueName *ValName) {
> -  StringRef Name = ValName->getKey();
> -
> +static Intrinsic::ID lookupIntrinsicID(StringRef Name) {
>    ArrayRef<const char *> NameTable = findTargetSubtable(Name);
>    int Idx = Intrinsic::lookupLLVMIntrinsicByName(NameTable, Name);
>    if (Idx == -1)
> @@ -508,12 +507,14 @@
>  }
>  
>  void Function::recalculateIntrinsicID() {
> -  const ValueName *ValName = this->getValueName();
> -  if (!ValName || !isIntrinsic()) {
> +  StringRef Name = getName();
> +  if (!Name.startswith("llvm.")) {
> +    HasLLVMReservedName = false;
>      IntID = Intrinsic::not_intrinsic;
>      return;
>    }
> -  IntID = lookupIntrinsicID(ValName);
> +  HasLLVMReservedName = true;
> +  IntID = lookupIntrinsicID(Name);
>  }
>  
>  /// Returns a stable mangling for the type specified for use in the name
> Index: include/llvm/IR/GlobalValue.h
> ===================================================================
> --- include/llvm/IR/GlobalValue.h
> +++ include/llvm/IR/GlobalValue.h
> @@ -72,7 +72,7 @@
>          ValueType(Ty), Linkage(Linkage), Visibility(DefaultVisibility),
>          UnnamedAddrVal(unsigned(UnnamedAddr::None)),
>          DllStorageClass(DefaultStorageClass), ThreadLocal(NotThreadLocal),
> -        IntID((Intrinsic::ID)0U), Parent(nullptr) {
> +        IntID((Intrinsic::ID)0U), HasLLVMReservedName(false), Parent(nullptr) {
>      setName(Name);
>    }
>  
> @@ -128,7 +128,12 @@
>    /// 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;
> +  Intrinsic::ID IntID : 31;
> +
> +  /// True if the function's name starts with "llvm.".  This corresponds to the
> +  /// value of Function::isIntrinsic(), which may be true even if
> +  /// Function::intrinsicID() returns Intrinsic::not_intrinsic.
> +  bool HasLLVMReservedName : 1;
>  
>    unsigned getGlobalValueSubClassData() const {
>      return SubClassData;
> Index: include/llvm/IR/Function.h
> ===================================================================
> --- include/llvm/IR/Function.h
> +++ include/llvm/IR/Function.h
> @@ -137,7 +137,11 @@
>    /// The particular intrinsic functions which correspond to this value are
>    /// defined in llvm/Intrinsics.h.
>    Intrinsic::ID getIntrinsicID() const LLVM_READONLY { return IntID; }
> -  bool isIntrinsic() const { return getName().startswith("llvm."); }
> +
> +  /// isIntrinsic - Returns true if the function's name starts with "llvm.".
> +  /// It's possible for this function to return true while getIntrinsicID()
> +  /// returns Intrinsic::not_intrinsic!
> +  bool isIntrinsic() const { return HasLLVMReservedName; }
>  
>    /// \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