[PATCH] D57315: [opaque pointer types] Add a FunctionCallee wrapper type, and use it.

[David Blaikie via Phabricator via llvm-commits]

dblaikie accepted this revision.
dblaikie added inline comments.
This revision is now accepted and ready to land.


================
Comment at: llvm/examples/HowToUseJIT/HowToUseJIT.cpp:71-74
   Function *Add1F =
-    cast<Function>(M->getOrInsertFunction("add1", Type::getInt32Ty(Context),
-                                          Type::getInt32Ty(Context)));
+      Function::Create(FunctionType::get(Type::getInt32Ty(Context),
+                                         {Type::getInt32Ty(Context)}, false),
+                       Function::ExternalLinkage, "add1", M);
----------------
Does this translation (of a getOrInsert cast to Function to a Function::Create) work (not assert, etc) if the Function with that specific name and type already does exist? (the old code would succeed at the cast - the new code, does Function::Create succeed even if the function already exists?)


================
Comment at: llvm/include/llvm/IR/DerivedTypes.h:168
+      : FnTy(FnTy), Callee(Callee) {
+    assert((FnTy == nullptr) == (Callee == nullptr));
+  }
----------------
In the interim (until opaque pointers happen) would it be correct/reasonable to assert in here that the type of the Value is a pointer to the FnType?


================
Comment at: llvm/include/llvm/IR/DerivedTypes.h:171-172
+
+  // Allow implicit conversion from types which have a getFunctionType member
+  // (e.g. Function and InlineAsm).
+  template <typename T,
----------------
If it's only these two, overloads might be simpler/clearer? (though I'm not super fussed if you have a clear preference for this form)


================
Comment at: llvm/include/llvm/IR/DerivedTypes.h:178
+  FunctionCallee(std::nullptr_t) : FnTy(nullptr), Callee(nullptr) {}
+  FunctionCallee() : FnTy(nullptr), Callee(nullptr) {}
+
----------------
potentially use non static data member initializers and = default this default ctor? (& the ctor above could skip its member initializers since null would be the default)


================
Comment at: llvm/include/llvm/IR/Instructions.h:1563-1564
+                          Instruction *InsertBefore = nullptr) {
+    return Create(Func.getFunctionType(), Func.getCallee(), Args, NameStr,
+                  InsertBefore);
   }
----------------
Worth having these as overloads, or would it be simpler/reasonable to change callers who don't have a FunctionCallee, to pass the first two args as {x, y} (as a separate commit would be totally OK of course)


================
Comment at: llvm/lib/Target/AArch64/AArch64ISelLowering.cpp:11753-11755
+    FunctionCallee SecurityCheckCookie = M.getOrInsertFunction(
+        "__security_check_cookie", Type::getVoidTy(M.getContext()),
+        Type::getInt8PtrTy(M.getContext()));
----------------
This one in particular wasn't changed to use Function::Create - perhaps this answers my previous question about whether Function::Create is OK for an existing function (in the sense that this code suggests it is not OK? hence why this code still uses getOrInsert)


================
Comment at: llvm/lib/Target/AArch64/AArch64ISelLowering.cpp:11756
+        Type::getInt8PtrTy(M.getContext()));
+    if (Function *F = dyn_cast<Function>(SecurityCheckCookie.getCallee())) {
+      F->setCallingConv(CallingConv::Win64);
----------------
Previous code made this a cast, not a dyn_cast - any reason to change that? (that means this new condition probably isn't tested - no test ever fails this dyn_cast?)


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

https://reviews.llvm.org/D57315