[PATCH] D57170: [opaque pointer types] Pass function types to CallInst creation.

[James Y Knight via Phabricator via llvm-commits]

jyknight marked an inline comment as done.
jyknight added inline comments.


================
Comment at: llvm/lib/Transforms/IPO/DeadArgumentElimination.cpp:203
     } else {
-      NewCS = CallInst::Create(NF, Args, OpBundles, "", Call);
+      NewCS = CallInst::Create(NF->getFunctionType(), NF, Args, OpBundles, "",
+                               Call);
----------------
dblaikie wrote:
> Worth having CallInst::Create overloads that take Functions and pulls out their function type rather than having to do it explicitly in the callers? Or is there not enough use/need for it, produces too many combinatorial overloads or the like?
I actually have those APIs -- and they're quite widely used here -- especially for calls to the result of getIntrinsic. But, it looks like I forgot the overload that takes the bundles argument. I've fixed that in the next version of the patch.

I intentionally hadn't added an overload for InlineAsm, since that doens't really seem common enough to matter. But, now that I'll be adding a new type for {FunctionType*, Value*}, it can also be implicitly constructible from InlineAsm nodes, so that'll take care of those places as well.


================
Comment at: llvm/lib/Transforms/Instrumentation/MemorySanitizer.cpp:575-576
   /// pointers.
+  FunctionType *MsanMetadataPtrGetterFnTy;
   Value *MsanMetadataPtrForLoadN, *MsanMetadataPtrForStoreN;
+  FunctionType *MsanMetadataPtrGetterFixedSizeFnTy;
----------------
dblaikie wrote:
> This patch adds lots of *Ty variables - worth introducing some kind of named pair of Value* and Type* for situations like this? (maybe as a separate patch/cleanup)
That seems like a really good idea, actually. I'll rework the patch to see how that looks!

I'm really liking that idea not so much because of the changes in this patch, but because in the future, when pointer bitcasts are gone, getOrInsertFunction will always return the Function itself. And even if it continues to declare a return-type of Constant*, I bet people will be do cast<Function>(...) just to avoid separately holding onto the type, which will be ever-so-subtly broken!


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D57170/new/

https://reviews.llvm.org/D57170