This is an archive of the discontinued LLVM Phabricator instance.

[llvm][NFC] Replace CallSite with CallBase in Inliner
ClosedPublic

Authored by mtrofin on Apr 9 2020, 11:32 AM.

Details

Summary

*Almost* all uses are replaced. Left FIXMEs for the two sites that
require refactoring outside of Inliner, to scope this patch.

Diff Detail

Event Timeline

mtrofin created this revision.Apr 9 2020, 11:32 AM
Herald added a project: Restricted Project. · View Herald TranscriptApr 9 2020, 11:32 AM
dblaikie accepted this revision.Apr 9 2020, 11:55 AM
dblaikie added a subscriber: dblaikie.

Looks good!

llvm/lib/Transforms/IPO/Inliner.cpp
949

Use 'auto *' here, now that CS is a pointer type

1157

Side note (not related to this patch (saw some other instances while reviewing this too, though) - but might be handy for reference otherwise) - generally for a locally used lambda like this (especially a lambda only used within its expression) I'd encourage using [&] rather than an explicit capture list - like any other nested scope, it should have access to all the enclosing variables implicitly.

This revision is now accepted and ready to land.Apr 9 2020, 11:55 AM
mtrofin updated this revision to Diff 256353.Apr 9 2020, 12:11 PM
mtrofin marked 3 inline comments as done.

fix Harbormaster lint failure

llvm/lib/Transforms/IPO/Inliner.cpp
1157

Ack - subsequent change. There are also naming inconsistencies (for (int i, instead of int I, etc))

dblaikie added inline comments.Apr 9 2020, 12:14 PM
llvm/lib/Transforms/IPO/Inliner.cpp
520

Yeah - this looks like Harbormaster just couldn't tell you didn't introduce this problem (the 'message' name not following LLVM naming conventions) - so I'd generally advise ignoring such a suggestion, or fixing it in an independent commit - no big deal/wouldn't bother fussing about it if you'd rather just commit it together this time.

What is the convention here? Pointer CallBase* vs CallBase& ?

What is the convention here? Pointer CallBase* vs CallBase& ?

I'd say "Same as other llvm::Instructions" - and I /assume/ the answer there is that they're still predominantly handled as pointers, rather than references?

Seems for /literally/ Instruction*/Instruction& (as opposed to all possible types of Instructions - harder to search/gather data for all of them) it's about 7:1 in favor of *, but that's hard to evaluate/compare - some of those may need to be pointers because null is an option.

Evidently references to Instructions aren't totally unheard of, so the preference "use references when something can't be null" could be observed here. A pretty clear endorsement of this is that "instructions(F)" gives you an iterator over Instruction&, not over Instruction*.

But yeah - given the current codebase I think it's dealer's choice & not something I'd push strongly either way in review personally. (but I don't do lots of Instruction handling, so maybe some other folks have stronger ideas about which direction this sort of thing should be going in)

What is the convention here? Pointer CallBase* vs CallBase& ?

I'd say "Same as other llvm::Instructions" - and I /assume/ the answer there is that they're still predominantly handled as pointers, rather than references?

Seems for /literally/ Instruction*/Instruction& (as opposed to all possible types of Instructions - harder to search/gather data for all of them) it's about 7:1 in favor of *, but that's hard to evaluate/compare - some of those may need to be pointers because null is an option.

Evidently references to Instructions aren't totally unheard of, so the preference "use references when something can't be null" could be observed here. A pretty clear endorsement of this is that "instructions(F)" gives you an iterator over Instruction&, not over Instruction*.

This seems like a reasonable guideline, though - i.e. using references in function signatures where the contract is the parameter must be non-null. Looking at the refactored sites, there are a few spots where the caller is assumed to pass a valid CallSite. While that wasn't checked, now may be a good time to have that signal.

Example: setInlineRemark

But yeah - given the current codebase I think it's dealer's choice & not something I'd push strongly either way in review personally. (but I don't do lots of Instruction handling, so maybe some other folks have stronger ideas about which direction this sort of thing should be going in)

mtrofin updated this revision to Diff 256360.Apr 9 2020, 1:00 PM

CallBase* -> CallBase& in places where the contract is that the value can't be null.

mtrofin marked an inline comment as done.Apr 9 2020, 1:02 PM
davidxl accepted this revision.Apr 9 2020, 1:17 PM

The old CallSite based interface is a mixed bag -- it is passed by value, but behaves like a reference sometimes but also supports 'null/boolean' check which is pointer like.

looks good

This revision was automatically updated to reflect the committed changes.