This is an archive of the discontinued LLVM Phabricator instance.

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

Authored by jyknight on Jan 27 2019, 8:45 PM.

Details

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.

Event Timeline

jyknight created this revision.Jan 27 2019, 8:45 PM
dblaikie accepted this revision.Jan 28 2019, 1:55 PM
dblaikie added inline comments.
llvm/examples/HowToUseJIT/HowToUseJIT.cpp
71–74

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

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

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

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

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

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

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

This revision is now accepted and ready to land.Jan 28 2019, 1:55 PM
jyknight marked 11 inline comments as done.Jan 31 2019, 10:49 AM
jyknight added inline comments.
llvm/examples/HowToUseJIT/HowToUseJIT.cpp
71–74

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

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

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

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

llvm/include/llvm/IR/Instructions.h
1563–1564

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

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

llvm/lib/CodeGen/MIRParser/MIRParser.cpp
274

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

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

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

11756

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.

This revision was automatically updated to reflect the committed changes.