This is an archive of the discontinued LLVM Phabricator instance.

[opaque pointer types] Pass function types to CallInst creation.
ClosedPublic

Authored by jyknight on Jan 24 2019, 10:53 AM.

Details

Summary

This cleans up all CallInst creation in LLVM to explicitly pass a
function type rather than deriving it from the pointer's element-type.

Event Timeline

jyknight created this revision.Jan 24 2019, 10:53 AM
dblaikie accepted this revision.Jan 24 2019, 1:18 PM

Looks pretty good - thanks!

llvm/lib/Transforms/IPO/DeadArgumentElimination.cpp
203

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?

llvm/lib/Transforms/Instrumentation/MemorySanitizer.cpp
575–576

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)

This revision is now accepted and ready to land.Jan 24 2019, 1:18 PM
jyknight marked an inline comment as done.Jan 24 2019, 2:45 PM
jyknight added inline comments.
llvm/lib/Transforms/IPO/DeadArgumentElimination.cpp
203

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.

llvm/lib/Transforms/Instrumentation/MemorySanitizer.cpp
575–576

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!

jyknight updated this revision to Diff 183796.Jan 27 2019, 8:48 PM

Update after creating FunctionCallee in D57315.

Sounds good - thanks! (skimmed this after the rebasing changes, so I haven't looked as in depth as the first pass - but assuming it's only the refactoring to go along with the FunctionCallee and other comment I made, I'm good with it)

This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptFeb 1 2019, 12:43 PM