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

[James Y Knight via Phabricator via llvm-commits]

jyknight marked 11 inline comments as done.
jyknight added inline comments.


================
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);
----------------
dblaikie wrote:
> 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?)
The old code could succeed at the cast (when the signature matches) -- but would then proceed to add more basic blocks to a function that may have already been created! That's certainly no good.

Function::Create will unconditionally create a brand new function, giving it a different name, if the name is already in use. That may or may not be a reasonable thing to do, depending on the circumstance.

There's two such changes in non-example/unittest code, I've left a comment on each.


================
Comment at: llvm/include/llvm/IR/DerivedTypes.h:168
+      : FnTy(FnTy), Callee(Callee) {
+    assert((FnTy == nullptr) == (Callee == nullptr));
+  }
----------------
dblaikie wrote:
> 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?
I believe it would be correct. However, I don't have a definition of Value here, only a forward-decl.

There's already an assert in CallInst::Create that the type of the value matches the function type, which I think will catch the problem soon enough.


================
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,
----------------
dblaikie wrote:
> 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)
Explicit overloads here are tricky, because this file doesn't include Function.h and InlineAsm.h. And I can't put them in InlineAsm and Function because it's a conversion from pointer to those types, not from the type itself.

So, this seemed the simplest way to express it.


================
Comment at: llvm/include/llvm/IR/DerivedTypes.h:178
+  FunctionCallee(std::nullptr_t) : FnTy(nullptr), Callee(nullptr) {}
+  FunctionCallee() : FnTy(nullptr), Callee(nullptr) {}
+
----------------
dblaikie wrote:
> 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)
Good idea -- I'd forgotten those were allowed.


================
Comment at: llvm/include/llvm/IR/Instructions.h:1563-1564
+                          Instruction *InsertBefore = nullptr) {
+    return Create(Func.getFunctionType(), Func.getCallee(), Args, NameStr,
+                  InsertBefore);
   }
----------------
dblaikie wrote:
> 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)
Good question. I'll not change this until the migration is all done. Then it may be worth checking if there's much need.


================
Comment at: llvm/include/llvm/IR/Module.h:332
 
+  /// FIXME: fix function description.
   /// Look up the specified function in the module symbol table. Four
----------------
Oops, forgot this FIXME. :) Updated the comment.


================
Comment at: llvm/lib/CodeGen/MIRParser/MIRParser.cpp:274
+  Function *F =
+      Function::Create(FunctionType::get(Type::getVoidTy(Context), false),
+                       Function::ExternalLinkage, Name, M);
----------------
This is okay because it only gets called after line 291 determines that there's no existing function of that name.


================
Comment at: llvm/lib/CodeGen/MachineOutliner.cpp:1107
   LLVMContext &C = M.getContext();
-  Function *F = dyn_cast<Function>(
-      M.getOrInsertFunction(NameStream.str(), Type::getVoidTy(C)));
-  assert(F && "Function was null!");
+  Function *F =
+      Function::Create(FunctionType::get(Type::getVoidTy(C), false),
----------------
The name of this function is irrelevant -- the code is creating both function and call, so if Function::Create renames it, that'd be fine. Additionally, the code above intended to choose a unique name already.


================
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()));
----------------
dblaikie wrote:
> 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)
This uses getOrInsert because it's referencing an externally-provided function, not defining a new function.



================
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);
----------------
dblaikie wrote:
> 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?)
There are many instances like this in the codebase, where it dyn_cast's the thing returned from getOrInsertFunction to add add various attributes to it.

I do not like this scheme at all and would like to get rid of that idiom, but I don't want to fix it all of that as part of this particular change.

The change here is simply to make this code use the typical idiom.


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

https://reviews.llvm.org/D57315