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

Do not make local copies of inline fns in debug mode #76896

Merged
merged 3 commits into from
Jan 8, 2021
Merged

Do not make local copies of inline fns in debug mode #76896

merged 3 commits into from
Jan 8, 2021

Conversation

spastorino
Copy link
Member

@spastorino spastorino commented Sep 18, 2020 •
edited by wesleywiser
Loading

r? @wesleywiser

cc @rust-lang/wg-incr-comp

If this is correct it supersedes #76889

Related to #54089

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Sep 18, 2020
@spastorino
Copy link
Member Author

@bors try @rust-timer queue

@rust-timer
Copy link
Collaborator

Awaiting bors try build completion

@bors
Copy link
Contributor

bors commented Sep 18, 2020

⌛ Trying commit 750d026f9f6561d9aab7019699f4f2617a8fde97 with merge 0128011c7686d53eca2f1f38b1187109c8c1d734...

@spastorino spastorino changed the title Do not inline in debug builds Do not make local copies of inline fns in debug mode Sep 18, 2020
@bors
Copy link
Contributor

bors commented Sep 18, 2020

☀️ Try build successful - checks-actions, checks-azure
Build commit: 0128011c7686d53eca2f1f38b1187109c8c1d734 (0128011c7686d53eca2f1f38b1187109c8c1d734)

@rust-timer
Copy link
Collaborator

Queued 0128011c7686d53eca2f1f38b1187109c8c1d734 with parent 2cbc570, future comparison URL.

@rust-timer
Copy link
Collaborator

Finished benchmarking try commit (0128011c7686d53eca2f1f38b1187109c8c1d734): 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

@spastorino spastorino changed the title Do not make local copies of inline fns in debug mode [WIP] Do not make local copies of inline fns in debug mode Sep 19, 2020
@spastorino spastorino force-pushed the codegen-inline-fns2 branch 2 times, most recently from 5ad917c to 72559c9 Compare September 21, 2020 15:15
@spastorino spastorino mentioned this pull request Sep 22, 2020
Closed
@crlf0710
Copy link
Member

crlf0710 commented Oct 8, 2020

Triage: Is this ready for review? There's still "[WIP]" in the PR title.

@spastorino
Copy link
Member Author

@crlf0710 will be working on this soon :).

@spastorino
Copy link
Member Author

@bors try @rust-timer queue

@rust-timer
Copy link
Collaborator

Awaiting bors try build completion

@spastorino spastorino changed the title [WIP] Do not make local copies of inline fns in debug mode Do not make local copies of inline fns in debug mode Oct 19, 2020
@bors
Copy link
Contributor

bors commented Oct 19, 2020

⌛ Trying commit c750d454122ecb66ea0fa99c34b6e92d3bd4f502 with merge ea5a68ada90e66a25730add973e96ac969449dc8...

@bors
Copy link
Contributor

bors commented Oct 19, 2020

☀️ Try build successful - checks-actions, checks-azure
Build commit: ea5a68ada90e66a25730add973e96ac969449dc8 (ea5a68ada90e66a25730add973e96ac969449dc8)

@rust-timer
Copy link
Collaborator

Queued ea5a68ada90e66a25730add973e96ac969449dc8 with parent a85e949, future comparison URL.

@rust-timer
Copy link
Collaborator

Finished benchmarking try commit (ea5a68ada90e66a25730add973e96ac969449dc8): 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

@wesleywiser
Copy link
Member

@bors r+

@bors
Copy link
Contributor

bors commented Oct 22, 2020

📌 Commit c750d454122ecb66ea0fa99c34b6e92d3bd4f502 has been approved by wesleywiser

@bors bors removed the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Oct 22, 2020
@bors bors added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Dec 18, 2020
@oli-obk
Copy link
Contributor

oli-obk commented Dec 18, 2020 •
edited
Loading

The failure looks legit. The output of run-make-fulldeps\coverage-reports changed.

@davidtwco davidtwco added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Dec 21, 2020
@andjo403 andjo403 mentioned this pull request Jan 6, 2021
Merged
Dylan-DPC-zz pushed a commit to Dylan-DPC-zz/rust that referenced this pull request Jan 7, 2021
@Dylan-DPC
Rollup merge of rust-lang#80761 - andjo403:generic_coverage, r=wesley…
38ccca7 
…wiser

handle generic trait methods in coverage-report tests

also make the generic function pattern more specific and remove the extra $ that fails the matching.

r? `@wesleywiser`
as this was failing the test of rust-lang#76896
JohnTitor added a commit to JohnTitor/rust that referenced this pull request Jan 8, 2021
@JohnTitor
Rollup merge of rust-lang#80761 - andjo403:generic_coverage, r=spasto…
c851ae9 
…rino

handle generic trait methods in coverage-report tests

also make the generic function pattern more specific and remove the extra $ that fails the matching.

r? `@wesleywiser`
as this was failing the test of rust-lang#76896
@andjo403
Copy link
Contributor

andjo403 commented Jan 8, 2021

The fix for the failure is merged now so this PR is ready for merge

@spastorino
Copy link
Member Author

Now that the coverage fix is merged ...

@bors r=davidtwco,wesleywiser

@bors
Copy link
Contributor

bors commented Jan 8, 2021

💡 This pull request was already approved, no need to approve it again.

@bors
Copy link
Contributor

bors commented Jan 8, 2021

📌 Commit 07a5982 has been approved by davidtwco,wesleywiser

@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-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Jan 8, 2021
@spastorino
Copy link
Member Author

@bors retry

@bors
Copy link
Contributor

bors commented Jan 8, 2021

⌛ Testing commit 07a5982 with merge ddf2cc7...

@bors
Copy link
Contributor

bors commented Jan 8, 2021

☀️ Test successful - checks-actions
Approved by: davidtwco,wesleywiser
Pushing ddf2cc7 to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Jan 8, 2021
@bors bors merged commit ddf2cc7 into rust-lang:master Jan 8, 2021
@rustbot rustbot added this to the 1.51.0 milestone Jan 8, 2021
@wesleywiser
Copy link
Member

I'll repost an earlier comment I left on this PR since it's collapsed now:

A note to the perf triage team

We (the incremental compilation working group) would like to go ahead and land this PR even though there are some regressions. We believe the improvements out-weight the regressions because the improvements are to real world crates while most of the regressions are to synthetic benchmarks or a result of other issues with CGU partitioning not caused by this PR.

I've written a much more through analysis of the perf results here. Please take a look if you'd like more information!

If you disagree, this PR can still be backed out as nothing else depends on it. Thanks!

This was referenced Jan 11, 2021
Closed
Closed
@alexcrichton
Copy link
Member

I've bisected a regression in stdarch's CI on the wasm target to this PR (opened an issue at #80916), and after reviewing this I don't think that this is the right change to make. It looks like the PR's title, not inlining functions in debug mode, already happens before this PR. This PR seems to only change inline(always) functions to not get inlined in debug mode.

I've historically been under the assumption, however, that inline(always) is, well, always inlined. That can sometimes be important for ABI reasons (e.g. I've seen backtrace-like scenarios where libraries generating a backtrace want to guarantee that a function is erased from the call-stack). Additionally with SIMD-related things LLVM's codegen is very particular about what attributes get applied where and how it all works out. Currently wasm SIMD relies on all the stars aligning which means that internally stdarch relies on inline(always) being always inlined.

This just landed so there's plenty of time to figure out what to do though, and I'd be happy to converse either here or at #80916

spastorino added a commit to spastorino/rust that referenced this pull request Jan 11, 2021
@spastorino
Revert "Auto merge of rust-lang#76896 - spastorino:codegen-inline-fns…
82d0c59 
…2, r=davidtwco,wesleywiser"

This reverts commit ddf2cc7, reversing
changes made to 937f629.
Dylan-DPC-zz pushed a commit to Dylan-DPC-zz/rust that referenced this pull request Jan 13, 2021
@Dylan-DPC
Rollup merge of rust-lang#80922 - spastorino:revert-inline-always-in-…
d64e703 
…debug, r=wesleywiser

Revert "Auto merge of rust-lang#76896 - spastorino:codegen-inline-fns2

This reverts commit ddf2cc7, reversing
changes made to 937f629.

As `@alexcrichton` pointed out in rust-lang#80916 there's a problem with the taken approach.
@Dylan-DPC-zz Dylan-DPC-zz mentioned this pull request Jan 13, 2021
Merged
bors added a commit to rust-lang-ci/rust that referenced this pull request Jan 13, 2021
@bors
Auto merge of rust-lang#80960 - Dylan-DPC:rollup-89tri8x, r=Dylan-DPC
fc93e47 
Rollup of 10 pull requests

Successful merges:

 - rust-lang#78901 (diagnostics: Note capturing closures can't be coerced to fns)
 - rust-lang#79588 (Provide more information for HRTB lifetime errors involving closures)
 - rust-lang#80232 (Remove redundant def_id lookups)
 - rust-lang#80662 (Added support for i386-unknown-linux-gnu and i486-unknown-linux-gnu)
 - rust-lang#80736 (use Once instead of Mutex to manage capture resolution)
 - rust-lang#80796 (Update to LLVM 11.0.1)
 - rust-lang#80859 (Fix --pretty=expanded with --remap-path-prefix)
 - rust-lang#80922 (Revert "Auto merge of rust-lang#76896 - spastorino:codegen-inline-fns2)
 - rust-lang#80924 (Fix rustdoc --test-builder argument parsing)
 - rust-lang#80935 (Rename `rustc_middle::lint::LevelSource` to `LevelAndSource`)

Failed merges:

r? `@ghost`
`@rustbot` modify labels: rollup
@wesleywiser wesleywiser mentioned this pull request Jan 14, 2021
Merged
@Mark-Simulacrum Mark-Simulacrum mentioned this pull request Jan 26, 2021
Merged
@tgnottingham tgnottingham mentioned this pull request Sep 3, 2023
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@wesleywiser wesleywiser wesleywiser left review comments

@oli-obk oli-obk oli-obk left review comments

@davidtwco davidtwco davidtwco approved these changes

Assignees

@davidtwco davidtwco

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.51.0
Development

Successfully merging this pull request may close these issues.