Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Fix setting inline hint based on InstanceDef::requires_inline #79106

Merged
merged 1 commit into from
Nov 19, 2020
Merged

Fix setting inline hint based on InstanceDef::requires_inline #79106

merged 1 commit into from
Nov 19, 2020

Conversation

tmiasko
Copy link
Contributor

@tmiasko tmiasko commented Nov 16, 2020 •
edited
Loading

For instances where InstanceDef::requires_inline is true, an attempt
is made to set an inline hint though a call to the inline function.
The attempt is ineffective, since all attributes will be usually removed
by the second call.

Fix the issue by applying the attributes only once, with user provided
attributes having a priority when provided.

Closes #79108.

@rust-highfive
Copy link
Collaborator

r? @oli-obk

(rust_highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Nov 16, 2020
@jonas-schievink
Copy link
Contributor

@bors try @rust-timer queue

@rust-timer
Copy link
Collaborator

Awaiting bors try build completion

@bors
Copy link
Contributor

bors commented Nov 16, 2020

⌛ Trying commit d21c26de759749074fa06ccb72acd35311920870 with merge 215cffaf2348b072fbf45e176795ee5c10645ea8...

@bors
Copy link
Contributor

bors commented Nov 16, 2020

☀️ Try build successful - checks-actions
Build commit: 215cffaf2348b072fbf45e176795ee5c10645ea8 (215cffaf2348b072fbf45e176795ee5c10645ea8)

@rust-timer
Copy link
Collaborator

Queued 215cffaf2348b072fbf45e176795ee5c10645ea8 with parent f5230fb, future comparison URL.

@rust-timer
Copy link
Collaborator

Finished benchmarking try commit (215cffaf2348b072fbf45e176795ee5c10645ea8): comparison url.

Benchmarking this pull request likely means that it is perf-sensitive, so we're automatically marking it as not fit for rolling up. Please note that if the perf results are neutral, you should likely undo the rollup=never given below by specifying rollup- to bors.

Importantly, though, if the results of this run are non-neutral do not roll this PR up -- it will mask other regressions or improvements in the roll up.

@bors rollup=never
@rustbot modify labels: +S-waiting-on-review -S-waiting-on-perf

@tmiasko
Fix setting inline hint based on InstanceDef::requires_inline
4ea25da 
For instances where `InstanceDef::requires_inline` is true, an attempt
is made to set an inline hint though a call to the `inline` function.
The attempt is ineffective, since all attributes will be usually removed
by the second call.

Fix the issue by applying the attributes only once, with user provided
attributes having a priority when provided.
@tmiasko
Copy link
Contributor Author

tmiasko commented Nov 17, 2020 •
edited
Loading

The perf results look quite promising. This fixes a bug where inline hint was
intended to be set by default for some instances, but in fact wasn't. With a
fix, LLVM codegen takes longer, but the cost is recovered by rustc being
sligthly faster (-0.1% average instruction counts). The rutsc boostrap timings
are worse though with +1.7%.

@tmiasko tmiasko changed the title experiment: Add a hint based on InstanceDef::requires_inline unless overridden Fix setting inline hint based on InstanceDef::requires_inline Nov 17, 2020
@eddyb
Copy link
Member

eddyb commented Nov 17, 2020

LGTM, cc @nagisa @wesleywiser @alexcrichton

nagisa
nagisa reviewed Nov 17, 2020
View reviewed changes
compiler/rustc_codegen_llvm/src/attributes.rs
Attribute::AlwaysInline.unapply_llfn(Function, val);
Attribute::NoInline.unapply_llfn(Function, val);
}
None if requires_inline => Attribute::InlineHint.apply_llfn(Function, val),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What's the intended behaviour here when requires_inline == true and inline != None?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If its not something that can happen, maybe bug!() it?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Requires inline instances are mostly auto-generated. Closures are the exception, so it is possible but unlikely. As far as what should take priority, no idea what the intent was, if it was considered at all (although the order of calls would suggest respecting user provided attributes).

@oli-obk
Copy link
Contributor

oli-obk commented Nov 17, 2020

r? @nagisa

@rust-highfive rust-highfive assigned nagisa and unassigned oli-obk Nov 17, 2020
@nagisa
Copy link
Member

nagisa commented Nov 18, 2020

@bors r=nagisa,eddyb

@bors
Copy link
Contributor

bors commented Nov 18, 2020

📌 Commit 4ea25da has been approved by nagisa,eddyb

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Nov 18, 2020
@bors
Copy link
Contributor

bors commented Nov 18, 2020

⌛ Testing commit 4ea25da with merge d91d08d88d83f418d20b15d79d093c7d85644e96...

@bors
Copy link
Contributor

bors commented Nov 18, 2020

💥 Test timed out

@bors bors added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Nov 18, 2020
@wesleywiser
Copy link
Member

@bors retry

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Nov 18, 2020
@bors
Copy link
Contributor

bors commented Nov 18, 2020

⌛ Testing commit 4ea25da with merge 675f114...

@bors
Copy link
Contributor

bors commented Nov 19, 2020

☀️ Test successful - checks-actions
Approved by: nagisa,eddyb
Pushing 675f114 to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Nov 19, 2020
@bors bors merged commit 675f114 into rust-lang:master Nov 19, 2020
@rustbot rustbot added this to the 1.50.0 milestone Nov 19, 2020
@tmiasko tmiasko deleted the inline-hint branch November 19, 2020 02:41
@bors bors mentioned this pull request Nov 19, 2020
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nagisa nagisa nagisa left review comments

Assignees

@nagisa nagisa

Labels
merged-by-bors This PR was explicitly merged by bors. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Milestone
1.50.0
Development

Successfully merging this pull request may close these issues.

InstanceDef::requires_inline does not result in an inline hint
10 participants
@tmiasko @rust-highfive @jonas-schievink @rust-timer @bors @eddyb @oli-obk @nagisa @wesleywiser @rustbot