[PATCH] D22065: Don't invoke getName() from Function::isIntrinsic().
Justin Lebar via llvm-commits
llvm-commits at lists.llvm.org
Wed Jul 27 15:39:12 PDT 2016
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
>>
More information about the llvm-commits
mailing list