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

use indirect return for i128 and f128 on wasm32 #135534

Merged
merged 1 commit into from
Jan 17, 2025
Merged

use indirect return for i128 and f128 on wasm32 #135534

merged 1 commit into from
Jan 17, 2025

Conversation

folkertdev
Copy link
Contributor

fixes #135532

Based on https://github.com/WebAssembly/tool-conventions/blob/main/BasicCABI.md we now use an indirect return for i128, u128 and f128. That is what LLVM ended up doing anyway.

r? @bjorn3

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Jan 15, 2025
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@bjorn3
Copy link
Member

bjorn3 commented Jan 15, 2025

Given that I was the one who actually suggested this:

r? compiler

@rustbot rustbot assigned lcnr and unassigned bjorn3 Jan 15, 2025
@folkertdev folkertdev mentioned this pull request Jan 15, 2025
Closed
@lcnr
Copy link
Contributor

lcnr commented Jan 15, 2025

r? compiler

@rustbot rustbot assigned cjgillot and unassigned lcnr Jan 15, 2025
tgross35
tgross35 reviewed Jan 15, 2025
View reviewed changes
Copy link
Contributor

@tgross35 tgross35 left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Could this get a codegen test? I recently wrote a similar one that you could probably copy part of https://github.com/rust-lang/rust/blob/3557d39ce2c8ae3b2fb57e9d900ce47796f0a13e/tests/codegen/i128-x86-callconv.rs

@folkertdev
Copy link
Contributor Author

I added codegen tests for i128 and f128 based on your code. The tests for naked-asm-wasm (soon) will provide further coverage.

tgross35
tgross35 approved these changes Jan 15, 2025
View reviewed changes
Copy link
Contributor

@tgross35 tgross35 left a comment

Choose a reason for hiding this comment

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

r=me with green CI

@tgross35 tgross35 assigned tgross35 and unassigned cjgillot Jan 15, 2025
@folkertdev
Copy link
Contributor Author

folkertdev commented Jan 15, 2025 •
edited
Loading

@bors r=tgross35

@bors
Copy link
Collaborator

bors commented Jan 15, 2025

@folkertdev: 🔑 Insufficient privileges: Not in reviewers

@folkertdev
Copy link
Contributor Author

@bors r=tgross35

without the @ maybe? or is something else messed up?

@bors
Copy link
Collaborator

bors commented Jan 15, 2025

@folkertdev: 🔑 Insufficient privileges: Not in reviewers

@tgross35
Copy link
Contributor

Bors still requires permissions to r=somebody, a reviewer commenting r=me doesn’t actually do anything

@bors r+

@bors
Copy link
Collaborator

bors commented Jan 15, 2025

📌 Commit fef3dd4 has been approved by tgross35

It is now in the queue for this repository.

@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 15, 2025
@folkertdev
Copy link
Contributor Author

ah, I see this from time to time but the person actually doing r=some_name must also themselves have the privileges then. And then only with an actual delegate can I (without privileges) merge things. well, TIL.

jhpratt added a commit to jhpratt/rust that referenced this pull request Jan 16, 2025
@jhpratt
Rollup merge of rust-lang#135534 - folkertdev:fix-wasm-i128-f128, r=t…
0386232 
…gross35

use indirect return for `i128` and `f128` on wasm32

fixes rust-lang#135532

Based on https://github.com/WebAssembly/tool-conventions/blob/main/BasicCABI.md we now use an indirect return for  `i128`, `u128` and `f128`. That is what LLVM ended up doing anyway.

r? `@bjorn3`
@jhpratt jhpratt mentioned this pull request Jan 16, 2025
Closed
bors added a commit to rust-lang-ci/rust that referenced this pull request Jan 16, 2025
@bors
Auto merge of rust-lang#135567 - jhpratt:rollup-eu4os4b, r=jhpratt
4dd5578 
Rollup of 5 pull requests

Successful merges:

 - rust-lang#134286 (Enable `unreachable_pub` lint in core)
 - rust-lang#135249 (Fix overflows in the implementation of `overflowing_literals` lint's help)
 - rust-lang#135534 (use indirect return for `i128` and `f128` on wasm32)
 - rust-lang#135556 (Clarify note in `std::sync::LazyLock` example)
 - rust-lang#135560 (Update `compiler-builtins` to 0.1.144)

r? `@ghost`
`@rustbot` modify labels: rollup
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Jan 16, 2025
@matthiaskrgr
Rollup merge of rust-lang#135534 - folkertdev:fix-wasm-i128-f128, r=t…
91fd468 
…gross35

use indirect return for `i128` and `f128` on wasm32

fixes rust-lang#135532

Based on https://github.com/WebAssembly/tool-conventions/blob/main/BasicCABI.md we now use an indirect return for  `i128`, `u128` and `f128`. That is what LLVM ended up doing anyway.

r? ``@bjorn3``
@matthiaskrgr matthiaskrgr mentioned this pull request Jan 16, 2025
Closed
bors added a commit to rust-lang-ci/rust that referenced this pull request Jan 16, 2025
@bors
Auto merge of rust-lang#135569 - matthiaskrgr:rollup-8vs4lv6, r=matth…
049d780 
…iaskrgr

Rollup of 4 pull requests

Successful merges:

 - rust-lang#135249 (Fix overflows in the implementation of `overflowing_literals` lint's help)
 - rust-lang#135534 (use indirect return for `i128` and `f128` on wasm32)
 - rust-lang#135556 (Clarify note in `std::sync::LazyLock` example)
 - rust-lang#135560 (Update `compiler-builtins` to 0.1.144)

r? `@ghost`
`@rustbot` modify labels: rollup
@matthiaskrgr
Copy link
Member

@bors r- rollup=iffy
#135569 (comment)

@bors bors 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-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Jan 16, 2025
@bjorn3
Copy link
Member

bjorn3 commented Jan 16, 2025

tests/codegen/i128-wasm32-callconv.rs and tests/codegen/f128-wasm32-callconv.rs will need to be updated.

@folkertdev
Copy link
Contributor Author

I think just adding -O should fix it? I don't think looking at the opt-level=0 output is all that useful.

I do wonder whether it makes sense to lint for codegen tests that don't set the opt level in some way, because I think in most cases you'd want the stability of the output that that provides (and some //@ all-opt-levels could be used when the test just looks at e.g. what symbols are even defined by llvm)

@rustbot ready

@rustbot rustbot 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 16, 2025
@bjorn3
Copy link
Member

bjorn3 commented Jan 16, 2025

@bors r=tgross35

@bors
Copy link
Collaborator

bors commented Jan 16, 2025

📌 Commit 702134a has been approved by tgross35

It is now in the queue for this repository.

@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 16, 2025
@bors
Copy link
Collaborator

bors commented Jan 17, 2025

⌛ Testing commit 702134a with merge bcd0683...

@bors
Copy link
Collaborator

bors commented Jan 17, 2025

☀️ Test successful - checks-actions
Approved by: tgross35
Pushing bcd0683 to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Jan 17, 2025
@bors bors merged commit bcd0683 into rust-lang:master Jan 17, 2025
7 checks passed
@rustbot rustbot added this to the 1.86.0 milestone Jan 17, 2025
@rust-timer
Copy link
Collaborator

Finished benchmarking commit (bcd0683): comparison URL.

Overall result: no relevant changes - no action needed

@rustbot label: -perf-regression

Instruction count

This benchmark run did not return any relevant results for this metric.

Max RSS (memory usage)

Results (secondary 2.7%)

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

mean range count
Regressions ❌
(primary)		 - - 0
Regressions ❌
(secondary)		 2.7% [2.7%, 2.7%] 1
Improvements ✅
(primary)		 - - 0
Improvements ✅
(secondary)		 - - 0
All ❌✅ (primary) - - 0

Cycles

Results (secondary 2.5%)

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

mean range count
Regressions ❌
(primary)		 - - 0
Regressions ❌
(secondary)		 2.5% [2.3%, 2.7%] 2
Improvements ✅
(primary)		 - - 0
Improvements ✅
(secondary)		 - - 0
All ❌✅ (primary) - - 0

Binary size

This benchmark run did not return any relevant results for this metric.

Bootstrap: 764.673s -> 763.784s (-0.12%)
Artifact size: 326.10 MiB -> 326.10 MiB (-0.00%)

@CryZe
Copy link
Contributor

CryZe commented Feb 12, 2025 •
edited
Loading

This seems to have broken the multivalue ABI where LLVM expects u128 / i128 to be passed as two i64 values. Though considering there's no real spec for the multivalue ABI yet, maybe that's fine? It definitely broke my multivalue target though.

@bjorn3
Copy link
Member

bjorn3 commented Feb 12, 2025

Are you patching rustc to add a multivalue target? If so you probably want to patch the ABI calculation too to use whatever multivalue ABI you are trying to use when compiling for this target.

@CryZe
Copy link
Contributor

CryZe commented Feb 12, 2025 •
edited
Loading

No, just a custom target spec json that activates the ABI (and is otherwise identical to the normal wasm32-unknown-unknown): https://github.com/LiveSplit/LiveSplitOne/blob/318ed8288fbf4cdd21e56fe0a778f0e579846f07/wasm32-multivalue.json#L19

@bjorn3
Copy link
Member

bjorn3 commented Feb 12, 2025

Even before this PR that would almost certainly not have been enough to fully match the multivalue ABI of clang. Pretty much every ABI change needs a change to the ABI calculation code in rustc. LLVM expects the frontend to do a bunch of ABI adjustments before generating LLVM IR. It only handles the lower half of the ABI implementation.

@CryZe
Copy link
Contributor

CryZe commented Feb 12, 2025 •
edited
Loading

Yeah it probably just "happened to work" because crates targeting the wasm32-unknown-unknown target already avoid C dependencies, so everything was purely Rust. Now Rust's libm implementations for u128 / i128 don't match what LLVM expect anymore (for this particular ABI) and it links it incorrectly.

It's probably fine not to fix anything in rustc, considering the ABI is considered heavily experimental anyway, with no real spec.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bjorn3 bjorn3 bjorn3 left review comments

@tgross35 tgross35 tgross35 approved these changes

Assignees

@tgross35 tgross35

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. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Milestone
1.86.0
Development

Successfully merging this pull request may close these issues.

ABI adjustment code for wasm doesn't handle 128bit ints and floats