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

[Justin Bogner via llvm-commits]

Justin Lebar <jlebar at google.com> writes:
> jlebar created this revision.
> jlebar added reviewers: joker-eph, majnemer, bogner.
> jlebar added a subscriber: llvm-commits.
>
> 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.".

Maybe we should call it "Function::hasLLVMReservedName()" or something
like that? It makes sense to call out intrinsics specifically in the
doxygen comment, but I find hasIntrinsicLikeName a bit confusing to read
in the callers.

> This change is good for a 1.5% e2e speedup compiling a large Eigen
> benchmark.
>
> http://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
> @@ -3624,7 +3624,7 @@
>        // Check to make sure that the "address of" an intrinsic function is never
>        // taken.
>        Assert(
> -          !F->isIntrinsic() ||
> +          !F->hasIntrinsicLikeName() ||
>                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
> @@ -481,7 +481,7 @@
>  
>  void Function::recalculateIntrinsicID() {
>    const ValueName *ValName = this->getValueName();
> -  if (!ValName || !isIntrinsic()) {
> +  if (!ValName || !ValName->first().startswith("llvm.")) {

I don't know how expensive it is to look up the name, but is the break
in abstraction here worthwhile just to avoid a double lookup? If this is
actually measurable I'd prefer to overload hasIntrinsicLikeName with one
that takes a StringRef, just to keep the code in one place.

As a side note, it seems a bit odd that this calls getValueName()
instead of just using getName() - lookupIntrinsicID immediately extracts
the name anyway.

>      IntID = Intrinsic::not_intrinsic;
>      return;
>    }
> 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->hasIntrinsicLikeName())
>          for (auto &Op : I.operands())
>            if (auto *V = dyn_cast_or_null<MetadataAsValue>(Op))
>              if (MDNode *N = dyn_cast<MDNode>(V->getMetadata()))
> @@ -3377,7 +3377,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->hasIntrinsicLikeName())
>          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
> +        // hasIntrinsicLikeName?  The latter is conservative.
>          auto *CalledFn =
>              dyn_cast<Function>(CS.getCalledValue()->stripPointerCasts());
> -        if (CalledFn && ((CalledFn->isIntrinsic() && CS.doesNotThrow()) ||
> -                         CS.isInlineAsm()))
> +        if (CalledFn &&
> +            ((CalledFn->hasIntrinsicLikeName() && CS.doesNotThrow()) ||
> +             CS.isInlineAsm()))

As you note, this is a little questionable.

>            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 hasIntrinsicLikeName() 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
>