Don't invoke getName() from Function::isIntrinsic().

[jlebar]

Summary

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.

[vsk]

I think @escha has tried something like this in the past, and would be a good reviewer.

Does having an intrinsic-like name carry any special meaning in llvm IR? I haven't really touched this stuff, but ISTM that behaving differently when you see an intrinsic-like name is suspicious.

include/llvm/IR/Function.h

141 ↗

(On Diff #62949)

Just assert this?

lib/IR/AsmWriter.cpp

3378 ↗

(On Diff #62949)

This chain of "if's" looks pretty familiar. Do you think it's worth pulling into a helper, e.g "MapOverIntrinsicMDValues" (in a separate commit)?

[jlebar]

Does having an intrinsic-like name carry any special meaning in llvm IR? I haven't really touched this stuff, but ISTM that behaving differently when you see an intrinsic-like name is suspicious.

Names starting with "llvm." are reserved for llvm's internal use, past, present, and future. So checking in the verifier that you don't take the address of any function named llvm. seems reasonable to me. The changes in AsmWriter are a bit more suspect: We're saying that you're allowed to attach metadata to any function named "llvm.". This seems pretty harmless, and is in fact useful if you wanted to emit IR for (say) a future version of llvm.

The changes to WinEHPrepare I'm less confident in. But I had a great deal of difficulty making the WinEH tests work with "real" intrinsics, and it's a conservative change, so I think it's OK (though certainly not ideal).

include/llvm/IR/Function.h

141 ↗

(On Diff #62949)

I was trying not to pull in Intrinsics.h -- otherwise I'd just return Intrinsic::not_intrinsic.

lib/IR/AsmWriter.cpp

3378 ↗

(On Diff #62949)

It might be worth someone doing that cleanup, yes. :)

[jlebar]

Apply Justin Bogner's suggestions.

In particular, the suggested change to Function::recalculateIntrinsicID seems
to have no performance impact.

llvm/trunk/include/llvm/IR/Function.h

public:
 
   /// getIntrinsicID - This method returns the ID number of the specified
   /// function, or Intrinsic::not_intrinsic if the function is not an
   /// intrinsic, or if the pointer is null.  This value is always defined to be
   /// zero to allow easy checking for whether a function is intrinsic or not.
   /// 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
   /// if the name of this function does not match an intrinsic in that header.
   /// Note, this method does not need to be called directly, as it is called
   /// from Value::setName() whenever the name of this function changes.
   void recalculateIntrinsicID();
 

llvm/trunk/lib/CodeGen/WinEHPrepare.cpp

for (BasicBlock *BB : BlocksInFunclet) {
         Value *FuncletBundleOperand = nullptr;
         if (auto BU = CS.getOperandBundle(LLVMContext::OB_funclet))
           FuncletBundleOperand = BU->Inputs.front();
 
         if (FuncletBundleOperand == FuncletPad)
           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()) ||
           continue;
 
         // This call site was not part of this funclet, remove it.
         if (CS.isInvoke()) {
           // Remove the unwind edge if it was an invoke.
           removeUnwindEdge(BB);
           // Get a pointer to the new call.
           BasicBlock::iterator CallI =

llvm/trunk/lib/IR/AsmWriter.cpp

for (auto &I : BB)
       processInstructionMetadata(I);
   }
 }
 
 void SlotTracker::processInstructionMetadata(const Instruction &I) {
   // 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()))
               CreateMetadataSlot(N);
 
   // Process metadata attached to this instruction.
   SmallVector<std::pair<unsigned, MDNode *>, 4> MDs;
   I.getAllMetadata(MDs);
if (!STy->isLiteral()) {
       OS << " = type ";
       TP.printStructBody(STy, OS);
     }
 }
 
 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()))
               return true;
   return false;
 }
 
 void Value::print(raw_ostream &ROS, bool IsForDebug) const {

llvm/trunk/lib/IR/Function.cpp

static ArrayRef<const char *> findTargetSubtable(StringRef Name) {
   // We've either found the target or just fall back to the generic set, which
   // is always first.
   const auto &TI = It != Targets.end() && It->Name == Target ? *It : Targets[0];
   return makeArrayRef(&IntrinsicNameTable[1] + TI.Offset, TI.Count);
 }
 
 /// \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)
     return Intrinsic::not_intrinsic;
 
   // Intrinsic IDs correspond to the location in IntrinsicNameTable, but we have
   // an index into a sub-table.
   int Adjust = NameTable.data() - IntrinsicNameTable;
   Intrinsic::ID ID = static_cast<Intrinsic::ID>(Idx + Adjust);
 
   // If the intrinsic is not overloaded, require an exact match. If it is
   // overloaded, require a prefix match.
   bool IsPrefixMatch = Name.size() > strlen(NameTable[Idx]);
   return IsPrefixMatch == isOverloaded(ID) ? ID : Intrinsic::not_intrinsic;
 }
 
 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
 /// mangling scheme used by 'any' types in intrinsic signatures.  The mangling
 /// of named types is simply their name.  Manglings for unnamed types consist
 /// of a prefix ('p' for pointers, 'a' for arrays, 'f_' for functions)
 /// combined with the mangling of their component types.  A vararg function
 /// type will have a suffix of 'vararg'.  Since function types can contain

llvm/trunk/lib/IR/Verifier.cpp

for (unsigned i = 0, e = I.getNumOperands(); i != e; ++i) {
     if (!I.getOperand(i)->getType()->isFirstClassType()) {
       Assert(0, "Instruction operands must be first-class values!", &I);
     }
 
     if (Function *F = dyn_cast<Function>(I.getOperand(i))) {
       // 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(
           !F->isIntrinsic() || isa<CallInst>(I) ||
               F->getIntrinsicID() == Intrinsic::donothing ||
               F->getIntrinsicID() == Intrinsic::experimental_patchpoint_void ||
               F->getIntrinsicID() == Intrinsic::experimental_patchpoint_i64 ||
               F->getIntrinsicID() == Intrinsic::experimental_gc_statepoint,