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

[Justin Lebar via llvm-commits]

jlebar marked 2 inline comments as done.
jlebar added a comment.

> 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).


================
Comment at: include/llvm/IR/Function.h:141
@@ +140,3 @@
+  bool isIntrinsic() const {
+    // Intrinsic::not_intrinsic must be 0.
+    return IntID != 0;
----------------
vsk wrote:
> Just assert this?
I was trying not to pull in Intrinsics.h -- otherwise I'd just return Intrinsic::not_intrinsic.

================
Comment at: lib/IR/AsmWriter.cpp:3378
@@ -3377,3 +3377,3 @@
 static bool isReferencingMDNode(const Instruction &I) {
   if (const auto *CI = dyn_cast<CallInst>(&I))
     if (Function *F = CI->getCalledFunction())
----------------
vsk wrote:
> 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)?
It might be worth someone doing that cleanup, yes.  :)


http://reviews.llvm.org/D22065