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.

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

Various ABI refactorings #80594

Merged
merged 6 commits into from
Jan 24, 2021
Merged

Various ABI refactorings #80594

merged 6 commits into from
Jan 24, 2021

Conversation

bjorn3
Copy link
Member

@bjorn3 bjorn3 commented Jan 1, 2021

This includes changes to the rust abi and various refactorings that will hopefully make it easier to use the abi handling infrastructure of rustc in cg_clif. There are several refactorings that I haven't done. I am opening this draft PR to check that I haven't broken any non x86_64 architectures.

r? @ghost

@bjorn3 bjorn3 added A-codegen Area: Code generation T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. A-cranelift Things relevant to the [future] cranelift backend labels Jan 1, 2021
@bjorn3
Copy link
Member Author

bjorn3 commented Jan 1, 2021

The rust abi changes may have a perf impact.

@bors try @rust-timer queue

@rust-timer
Copy link
Collaborator

Awaiting bors try build completion.

@bors
Copy link
Contributor

bors commented Jan 1, 2021

⌛ Trying commit aa907768403aacd3b5750b496ad5823891176e3d with merge 075e7afe5cd922c1d7be743e49df45cb2d3646ad...

@rust-timer
Copy link
Collaborator

Awaiting bors try build completion.

@bors
Copy link
Contributor

bors commented Jan 1, 2021

☀️ Try build successful - checks-actions
Build commit: 075e7afe5cd922c1d7be743e49df45cb2d3646ad (075e7afe5cd922c1d7be743e49df45cb2d3646ad)

@rust-timer
Copy link
Collaborator

Queued 075e7afe5cd922c1d7be743e49df45cb2d3646ad with parent 206ee1e, future comparison URL.

@rustbot label: +S-waiting-on-perf

@rustbot rustbot added the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Jan 1, 2021
@bjorn3
Copy link
Member Author

bjorn3 commented Jan 1, 2021

Overall this is a slight (<0.5%) improvement.

@rust-timer
Copy link
Collaborator

Finished benchmarking try commit (075e7afe5cd922c1d7be743e49df45cb2d3646ad): 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 label: +S-waiting-on-review -S-waiting-on-perf

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-perf Status: Waiting on a perf run to be completed. labels Jan 1, 2021
…n return position

Abi::ScalarPair is only ever used for types that don't have a stable
layout anyway so this doesn't break any FFI. It does however reduce the
amount of special casing on the abi outside of the code responsible for
abi specific adjustments to the pass mode.
@bjorn3 bjorn3 marked this pull request as ready for review January 23, 2021 12:56
@bjorn3 bjorn3 changed the title [WIP] Various ABI refactorings Various ABI refactorings Jan 23, 2021
@bjorn3
Copy link
Member Author

bjorn3 commented Jan 23, 2021

I think this is enough to port cg_clif to the same abi handling infrastructure as cg_llvm.

Randomly assigning

r? @petrochenkov

compiler/rustc_middle/src/ty/layout.rs Outdated Show resolved Hide resolved
compiler/rustc_middle/src/ty/layout.rs Outdated Show resolved Hide resolved
@petrochenkov
Copy link
Contributor

I'm not familiar with this code and not sure what are the full implications of da0309c, but this looks reasonable.
r=me

@petrochenkov petrochenkov 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 Jan 23, 2021
@bjorn3
Copy link
Member Author

bjorn3 commented Jan 23, 2021

I am not sure about da0309c either, but it is probably the reason this PR improves performance and it removes a special case so I would like to see it landing, but I would like for someone who is more familiar with the abi handling to take a look.

@bjorn3 bjorn3 added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Jan 23, 2021
@petrochenkov
Copy link
Contributor

@bors r+

@bors
Copy link
Contributor

bors commented Jan 23, 2021

📌 Commit fa12fdb has been approved by petrochenkov

@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 Jan 23, 2021
@bors
Copy link
Contributor

bors commented Jan 24, 2021

⌛ Testing commit fa12fdb with merge 446cbc9...

@bors
Copy link
Contributor

bors commented Jan 24, 2021

☀️ Test successful - checks-actions
Approved by: petrochenkov
Pushing 446cbc9 to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Jan 24, 2021
@bors bors merged commit 446cbc9 into rust-lang:master Jan 24, 2021
@rustbot rustbot added this to the 1.51.0 milestone Jan 24, 2021
@bjorn3 bjorn3 deleted the abi_refactor3 branch January 24, 2021 14:29
@alexcrichton
Copy link
Member

I believe this has caused a breaking change in wasm32-unknown-unknown's ABI -- #81386

bors added a commit to rust-lang-ci/rust that referenced this pull request Nov 19, 2023
Ensure sanity of all computed ABIs

This moves the ABI sanity assertions from the codegen backend to the ABI computation logic. Sadly, due to past mistakes, we [have to](rust-lang#117351 (comment)) be able to compute a sane ABI for nonsensical function types like `extern "C" fn(str) -> str`.  So to make the sanity check pass we first need to make all ABI adjustment deal with unsized types... and we have no shared infrastructure for those adjustments, so that's a bunch of copy-paste. At least we have assertions failing loudly when one accidentally sets a different mode for an unsized argument.

To achieve this, this re-lands the parts of rust-lang#80594 that got reverted in rust-lang#81388.  To avoid breaking wasm ABI again, that ABI now explicitly opts-in to the (wrong, broken) ABI that we currently keep for backwards compatibility. That's still better than having *every* ABI use the wrong broken default!

Cc `@bjorn3`
Fixes rust-lang#115845
bors added a commit to rust-lang-ci/rust that referenced this pull request Nov 19, 2023
Ensure sanity of all computed ABIs

This moves the ABI sanity assertions from the codegen backend to the ABI computation logic. Sadly, due to past mistakes, we [have to](rust-lang#117351 (comment)) be able to compute a sane ABI for nonsensical function types like `extern "C" fn(str) -> str`.  So to make the sanity check pass we first need to make all ABI adjustment deal with unsized types... and we have no shared infrastructure for those adjustments, so that's a bunch of copy-paste. At least we have assertions failing loudly when one accidentally sets a different mode for an unsized argument.

To achieve this, this re-lands the parts of rust-lang#80594 that got reverted in rust-lang#81388.  To avoid breaking wasm ABI again, that ABI now explicitly opts-in to the (wrong, broken) ABI that we currently keep for backwards compatibility. That's still better than having *every* ABI use the wrong broken default!

Cc `@bjorn3`
Fixes rust-lang#115845
bors added a commit to rust-lang-ci/rust that referenced this pull request Nov 19, 2023
Ensure sanity of all computed ABIs

This moves the ABI sanity assertions from the codegen backend to the ABI computation logic. Sadly, due to past mistakes, we [have to](rust-lang#117351 (comment)) be able to compute a sane ABI for nonsensical function types like `extern "C" fn(str) -> str`.  So to make the sanity check pass we first need to make all ABI adjustment deal with unsized types... and we have no shared infrastructure for those adjustments, so that's a bunch of copy-paste. At least we have assertions failing loudly when one accidentally sets a different mode for an unsized argument.

To achieve this, this re-lands the parts of rust-lang#80594 that got reverted in rust-lang#81388.  To avoid breaking wasm ABI again, that ABI now explicitly opts-in to the (wrong, broken) ABI that we currently keep for backwards compatibility. That's still better than having *every* ABI use the wrong broken default!

Cc `@bjorn3`
Fixes rust-lang#115845
bors added a commit to rust-lang/miri that referenced this pull request Nov 21, 2023
Ensure sanity of all computed ABIs

This moves the ABI sanity assertions from the codegen backend to the ABI computation logic. Sadly, due to past mistakes, we [have to](rust-lang/rust#117351 (comment)) be able to compute a sane ABI for nonsensical function types like `extern "C" fn(str) -> str`.  So to make the sanity check pass we first need to make all ABI adjustment deal with unsized types... and we have no shared infrastructure for those adjustments, so that's a bunch of copy-paste. At least we have assertions failing loudly when one accidentally sets a different mode for an unsized argument.

To achieve this, this re-lands the parts of rust-lang/rust#80594 that got reverted in rust-lang/rust#81388.  To avoid breaking wasm ABI again, that ABI now explicitly opts-in to the (wrong, broken) ABI that we currently keep for backwards compatibility. That's still better than having *every* ABI use the wrong broken default!

Cc `@bjorn3`
Fixes rust-lang/rust#115845
lnicola pushed a commit to lnicola/rust-analyzer that referenced this pull request Apr 7, 2024
Ensure sanity of all computed ABIs

This moves the ABI sanity assertions from the codegen backend to the ABI computation logic. Sadly, due to past mistakes, we [have to](rust-lang/rust#117351 (comment)) be able to compute a sane ABI for nonsensical function types like `extern "C" fn(str) -> str`.  So to make the sanity check pass we first need to make all ABI adjustment deal with unsized types... and we have no shared infrastructure for those adjustments, so that's a bunch of copy-paste. At least we have assertions failing loudly when one accidentally sets a different mode for an unsized argument.

To achieve this, this re-lands the parts of rust-lang/rust#80594 that got reverted in rust-lang/rust#81388.  To avoid breaking wasm ABI again, that ABI now explicitly opts-in to the (wrong, broken) ABI that we currently keep for backwards compatibility. That's still better than having *every* ABI use the wrong broken default!

Cc `@bjorn3`
Fixes rust-lang/rust#115845
RalfJung pushed a commit to RalfJung/rust-analyzer that referenced this pull request Apr 27, 2024
Ensure sanity of all computed ABIs

This moves the ABI sanity assertions from the codegen backend to the ABI computation logic. Sadly, due to past mistakes, we [have to](rust-lang/rust#117351 (comment)) be able to compute a sane ABI for nonsensical function types like `extern "C" fn(str) -> str`.  So to make the sanity check pass we first need to make all ABI adjustment deal with unsized types... and we have no shared infrastructure for those adjustments, so that's a bunch of copy-paste. At least we have assertions failing loudly when one accidentally sets a different mode for an unsized argument.

To achieve this, this re-lands the parts of rust-lang/rust#80594 that got reverted in rust-lang/rust#81388.  To avoid breaking wasm ABI again, that ABI now explicitly opts-in to the (wrong, broken) ABI that we currently keep for backwards compatibility. That's still better than having *every* ABI use the wrong broken default!

Cc `@bjorn3`
Fixes rust-lang/rust#115845
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-codegen Area: Code generation A-cranelift Things relevant to the [future] cranelift backend 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. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants