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

[Justin Lebar via llvm-commits]

Thank you for the review, Justin.  I went ahead and applied your
suggestions.  I don't think the double-lookup is significant to
performance, and it's definitely cleaner, so I went with that.

On Wed, Jul 20, 2016 at 11:51 AM, Justin Bogner <mail at justinbogner.com> wrote:
> 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
>>