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

[jyknight]

Summary

The FunctionCallee type is effectively a {FunctionType*,Value*} pair,
and is a useful convenience to enable code to continue passing the
result of getOrInsertFunction() through to EmitCall, even once pointer
types lose their pointee-type.

Then:

• update the CallInst/InvokeInst instruction creation functions to

take a Callee,

• modify getOrInsertFunction to return FunctionCallee, and
• update all callers appropriately.

One area of particular note is the change to the sanitizer
code. Previously, they had been casting the result of
getOrInsertFunction to a Function* via
checkSanitizerInterfaceFunction, and storing that. That would report
an error if someone had already inserted a function declaraction with
a mismatching signature.

However, in general, LLVM allows for such mismatches, as
getOrInsertFunction will automatically insert a bitcast if
needed. As part of this cleanup, cause the sanitizer code to do the
same. (It will call its functions using the expected signature,
however they may have been declared.)

Finally, in a small number of locations, callers of
getOrInsertFunction actually were expecting/requiring that a brand
new function was being created. In such cases, I've switched them to
Function::Create instead.

[dblaikie]

llvm/examples/HowToUseJIT/HowToUseJIT.cpp

71–74 ↗

(On Diff #183795)

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

llvm/include/llvm/IR/DerivedTypes.h

168 ↗

(On Diff #183795)

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?

171–172 ↗

(On Diff #183795)

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)

178 ↗

(On Diff #183795)

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)

llvm/include/llvm/IR/Instructions.h

1563–1564 ↗

(On Diff #183795)

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)

llvm/lib/Target/AArch64/AArch64ISelLowering.cpp

11753–11755 ↗

(On Diff #183795)

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)

11756 ↗

(On Diff #183795)

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

[jyknight]

llvm/examples/HowToUseJIT/HowToUseJIT.cpp

71–74 ↗

(On Diff #183795)

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.

llvm/include/llvm/IR/DerivedTypes.h

168 ↗

(On Diff #183795)

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.

171–172 ↗

(On Diff #183795)

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.

178 ↗

(On Diff #183795)

Good idea -- I'd forgotten those were allowed.

llvm/include/llvm/IR/Instructions.h

1563–1564 ↗

(On Diff #183795)

Good question. I'll not change this until the migration is all done. Then it may be worth checking if there's much need.

llvm/include/llvm/IR/Module.h

332 ↗

(On Diff #183795)

Oops, forgot this FIXME. :) Updated the comment.

llvm/lib/CodeGen/MIRParser/MIRParser.cpp

274 ↗

(On Diff #183795)

This is okay because it only gets called after line 291 determines that there's no existing function of that name.

llvm/lib/CodeGen/MachineOutliner.cpp

1107 ↗

(On Diff #183795)

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.

llvm/lib/Target/AArch64/AArch64ISelLowering.cpp

11753–11755 ↗

(On Diff #183795)

This uses getOrInsert because it's referencing an externally-provided function, not defining a new function.

11756 ↗

(On Diff #183795)

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.

lib/CodeGen/CGExpr.cpp

void CodeGenFunction::EmitCfiSlowPathCheck(
   llvm::MDNode *Node = MDHelper.createBranchWeights((1U << 20) - 1, 1);
   BI->setMetadata(llvm::LLVMContext::MD_prof, Node);
 
   EmitBlock(CheckBB);
 
   bool WithDiag = !CGM.getCodeGenOpts().SanitizeTrap.has(Kind);
 
   llvm::CallInst *CheckCall;
-  llvm::Constant *SlowPathFn;
+  llvm::FunctionCallee SlowPathFn;
   if (WithDiag) {
     llvm::Constant *Info = llvm::ConstantStruct::getAnon(StaticArgs);
     auto *InfoPtr =
         new llvm::GlobalVariable(CGM.getModule(), Info->getType(), false,
                                  llvm::GlobalVariable::PrivateLinkage, Info);
     InfoPtr->setUnnamedAddr(llvm::GlobalValue::UnnamedAddr::Global);
     CGM.getSanitizerMetadata()->disableSanitizerForGlobal(InfoPtr);
 
     SlowPathFn = CGM.getModule().getOrInsertFunction(
         "__cfi_slowpath_diag",
         llvm::FunctionType::get(VoidTy, {Int64Ty, Int8PtrTy, Int8PtrTy},
                                 false));
     CheckCall = Builder.CreateCall(
         SlowPathFn, {TypeId, Ptr, Builder.CreateBitCast(InfoPtr, Int8PtrTy)});
   } else {
     SlowPathFn = CGM.getModule().getOrInsertFunction(
         "__cfi_slowpath",
         llvm::FunctionType::get(VoidTy, {Int64Ty, Int8PtrTy}, false));
     CheckCall = Builder.CreateCall(SlowPathFn, {TypeId, Ptr});
   }
 
-  CGM.setDSOLocal(cast<llvm::GlobalValue>(SlowPathFn->stripPointerCasts()));
+  CGM.setDSOLocal(
+      cast<llvm::GlobalValue>(SlowPathFn.getCallee()->stripPointerCasts()));
   CheckCall->setDoesNotThrow();
 
   EmitBlock(Cont);
 }
 
 // Emit a stub for __cfi_check function so that the linker knows about this
 // symbol in LTO mode.
 void CodeGenFunction::EmitCfiCheckStub() {