Page MenuHomePhabricator

[Inliner] Inlining should honor nobuiltin attributes
ClosedPublic

Authored by tejohnson on Feb 6 2020, 1:36 PM.

Details

Summary

Final patch in series to fix inlining between functions with different
nobuiltin attributes/options, which was specifically an issue in LTO.
See discussion on D61634 for background.

The prior patch in this series (D67923) enabled per-Function TLI
construction that identified the nobuiltin attributes.

Here I have allowed inlining to proceed if the callee's nobuiltins are a
subset of the caller's nobuiltins, but not in the reverse case, which
should be conservatively correct.

Diff Detail

Event Timeline

tejohnson created this revision.Feb 6 2020, 1:36 PM
Herald added a project: Restricted Project. · View Herald TranscriptFeb 6 2020, 1:36 PM

Apart from my other comment LGTM

llvm/lib/Transforms/IPO/PartialInlining.cpp
393

I'm having a hard time convincing myself that the lifetime requirements are correct here.
Passing a local variable GetTLI by address in return statement looks fishy.

It's similar to GetTTI so is seems correct, it's just hard to tell by looking at the code.

Same above and below.

nhaehnle removed a subscriber: nhaehnle.Feb 7 2020, 3:18 AM
tejohnson marked an inline comment as done.Feb 7 2020, 6:49 AM
tejohnson added inline comments.
llvm/lib/Transforms/IPO/PartialInlining.cpp
393

What's being returned is the bool result of the run() call, not the PartialInlinerImpl object, which doesn't survive past this function and therefore the GetTLI scope.

gchatelet accepted this revision.Feb 7 2020, 7:13 AM
gchatelet marked an inline comment as done.

Let's wait a bit for other reviewers to comment.

llvm/lib/Transforms/IPO/PartialInlining.cpp
393

Ha right, I got confused by the formatting and missed the run() on next line.

This revision is now accepted and ready to land.Feb 7 2020, 7:13 AM

@davidxl David can you take a look at it from the inliner side and let me know if it looks ok?

davidxl added inline comments.Feb 27 2020, 10:03 AM
llvm/include/llvm/Analysis/TargetLibraryInfo.h
267

This may be bad for performance -- as the inline instance will be optimized differently.

llvm/test/Transforms/Inline/X86/inline-no-builtin-compatible.ll
6 ↗(On Diff #242994)

Is this test x86 specific?

23 ↗(On Diff #242994)

why not directly check-not call?

tejohnson marked 3 inline comments as done.Feb 27 2020, 10:13 AM
tejohnson added inline comments.
llvm/include/llvm/Analysis/TargetLibraryInfo.h
267

Note that it won't be worse than head, which doesn't restrict the inlines based on nobuiltin attributes at all. We could also just disallow inlining completely between callers/callees with different nobuiltin attributes. But I was concerned that this would degrade performance too much by disallowing inlining in too many cases.

llvm/test/Transforms/Inline/X86/inline-no-builtin-compatible.ll
6 ↗(On Diff #242994)

Actually not, I can move to parent directory.

23 ↗(On Diff #242994)

Yeah that would be better, will change.

Address comments

davidxl added inline comments.Feb 27 2020, 10:20 AM
llvm/include/llvm/Analysis/TargetLibraryInfo.h
267

Perhaps add an additional parameter to the interface to allow superset behavior. Then in the inlineCost.cpp, add an internal option to specify whether strict attribute matching is required -- the default can be the current behavior -- allow inlining into superset.

tejohnson marked an inline comment as done.Feb 27 2020, 9:51 PM
tejohnson updated this revision to Diff 247165.Feb 27 2020, 9:53 PM

Add internal option to control superset check and test it

davidxl accepted this revision.Feb 27 2020, 9:57 PM

lgtm

gchatelet added inline comments.Feb 28 2020, 1:11 AM
llvm/include/llvm/Analysis/TargetLibraryInfo.h
267

Note that it won't be worse than head, which doesn't restrict the inlines based on nobuiltin attributes at all. We could also just disallow inlining completely between callers/callees with different nobuiltin attributes. But I was concerned that this would degrade performance too much by disallowing inlining in too many cases.

I agree disallowing inlining completely when nobuiltin differ would prevent inlining of basic memory functions entirely (memset, memcpy, etc..)

This revision was automatically updated to reflect the committed changes.