[PATCH] D22065: Don't invoke getName() from Function::isIntrinsic().

[Justin Bogner via llvm-commits]

Justin Lebar <jlebar at google.com> writes:
> getName() involves a hashtable lookup, so is expensive given how
> frequently isIntrinsic() is called.  (In particular, many users cast to
> IntrinsicInstr or one of its subclasses before calling
> getIntrinsicID().)
>
> This has an incidental functional change: Before, isIntrinsic() would
> return true for any function whose name started with "llvm.", even if it
> wasn't properly an intrinsic.  The new behavior seems more correct to
> me, because it's strange to say that isIntrinsic() is true, but
> getIntrinsicId() returns "not an intrinsic".
>
> Some callers want the old behavior -- they want to know whether the
> caller is a recognized intrinsic, or might be one in some other version
> of LLVM.  For them, we added Function::hasIntrinsicLikeName(), which
> checks whether the name starts with "llvm.".
>
> This change is good for a 1.5% e2e speedup compiling a large Eigen
> benchmark.
 ...
> jlebar updated this revision to Diff 65822.
> jlebar marked 4 inline comments as done.
> jlebar added a comment.
>
> Apply Justin Bogner's suggestions.

LGTM. Thanks!

> In particular, the suggested change to Function::recalculateIntrinsicID seems
> to have no performance impact.
>
>
> https://reviews.llvm.org/D22065
>
> Files:
>   include/llvm/IR/Function.h
>   lib/CodeGen/WinEHPrepare.cpp
>   lib/IR/AsmWriter.cpp
>   lib/IR/Function.cpp
>   lib/IR/Verifier.cpp
>
> Index: lib/IR/Verifier.cpp
> ===================================================================
> --- lib/IR/Verifier.cpp
> +++ lib/IR/Verifier.cpp
> @@ -3647,7 +3647,7 @@
>        // Check to make sure that the "address of" an intrinsic function is never
>        // taken.
>        Assert(
> -          !F->isIntrinsic() ||
> +          !F->hasLLVMReservedName() ||
>                i == (isa<CallInst>(I) ? e - 1 : isa<InvokeInst>(I) ? e - 3 : 0),
>            "Cannot take the address of an intrinsic!", &I);
>        Assert(
> Index: lib/IR/Function.cpp
> ===================================================================
> --- lib/IR/Function.cpp
> +++ lib/IR/Function.cpp
> @@ -488,9 +488,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 +506,11 @@
>  }
>  
>  void Function::recalculateIntrinsicID() {
> -  const ValueName *ValName = this->getValueName();
> -  if (!ValName || !isIntrinsic()) {
> +  if (!hasLLVMReservedName()) {
>      IntID = Intrinsic::not_intrinsic;
>      return;
>    }
> -  IntID = lookupIntrinsicID(ValName);
> +  IntID = lookupIntrinsicID(getName());
>  }
>  
>  /// Returns a stable mangling for the type specified for use in the name
> Index: lib/IR/AsmWriter.cpp
> ===================================================================
> --- lib/IR/AsmWriter.cpp
> +++ lib/IR/AsmWriter.cpp
> @@ -905,7 +905,7 @@
>    // Process metadata used directly by intrinsics.
>    if (const CallInst *CI = dyn_cast<CallInst>(&I))
>      if (Function *F = CI->getCalledFunction())
> -      if (F->isIntrinsic())
> +      if (F->hasLLVMReservedName())
>          for (auto &Op : I.operands())
>            if (auto *V = dyn_cast_or_null<MetadataAsValue>(Op))
>              if (MDNode *N = dyn_cast<MDNode>(V->getMetadata()))
> @@ -3378,7 +3378,7 @@
>  static bool isReferencingMDNode(const Instruction &I) {
>    if (const auto *CI = dyn_cast<CallInst>(&I))
>      if (Function *F = CI->getCalledFunction())
> -      if (F->isIntrinsic())
> +      if (F->hasLLVMReservedName())
>          for (auto &Op : I.operands())
>            if (auto *V = dyn_cast_or_null<MetadataAsValue>(Op))
>              if (isa<MDNode>(V->getMetadata()))
> Index: lib/CodeGen/WinEHPrepare.cpp
> ===================================================================
> --- lib/CodeGen/WinEHPrepare.cpp
> +++ lib/CodeGen/WinEHPrepare.cpp
> @@ -949,10 +949,14 @@
>            continue;
>  
>          // Skip call sites which are nounwind intrinsics or inline asm.
> +        //
> +        // FIXME: Should this check isIntrinsic() instead of
> +        // hasLLVMReservedName?  The latter is conservative.
>          auto *CalledFn =
>              dyn_cast<Function>(CS.getCalledValue()->stripPointerCasts());
> -        if (CalledFn && ((CalledFn->isIntrinsic() && CS.doesNotThrow()) ||
> -                         CS.isInlineAsm()))
> +        if (CalledFn &&
> +            ((CalledFn->hasLLVMReservedName() && CS.doesNotThrow()) ||
> +             CS.isInlineAsm()))
>            continue;
>  
>          // This call site was not part of this funclet, remove it.
> Index: include/llvm/IR/Function.h
> ===================================================================
> --- include/llvm/IR/Function.h
> +++ include/llvm/IR/Function.h
> @@ -137,7 +137,13 @@
>    /// 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."); }
> +  bool isIntrinsic() const {
> +    // Intrinsic::not_intrinsic must be 0.
> +    return IntID != 0;
> +  }
> +  /// Return true if the function's name starts with "llvm.".  All intrinsics
> +  /// have this prefix.
> +  bool hasLLVMReservedName() 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