-
Notifications
You must be signed in to change notification settings - Fork 12.9k
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
std: Get the standard library compiling for wasm64 #90382
Conversation
r? @yaahc (rust-highfive has picked a reviewer for you, use r? to override) |
|
As a heads up there are some sibling PRs which are not required to get pulled into libstd just yet but will likely want to get pulled in once they're merged:
This PR, however, I believe is still standalone and can be landed separately from those.
|
i think you should update platform-support.md to move wasm64 from |
@@ -24,7 +24,8 @@ pub const MIN_ALIGN: usize = 8; | |||
target_arch = "mips64", | |||
target_arch = "s390x", | |||
target_arch = "sparc64", | |||
target_arch = "riscv64" | |||
target_arch = "riscv64", | |||
target_arch = "wasm64", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This isn't something you need to fix in this PR, but it feels like these should use #[cfg(target_pointer_width = "64")]
and similar, and then just list any exceptions to that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If someone forgets to add an exception that may lead to a hard to track down bug.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this would still be a maintenance improvement even if code directly above this had a list of all target architectures and a compilation error for an unknown architecture.
@@ -276,7 +276,7 @@ target | std | host | notes | |||
`thumbv7a-pc-windows-msvc` | ? | | | |||
`thumbv7a-uwp-windows-msvc` | ✓ | | | |||
`thumbv7neon-unknown-linux-musleabihf` | ? | | Thumb2-mode ARMv7a Linux with NEON, MUSL | |||
`wasm64-unknown-unknown` | * | | WebAssembly | |||
`wasm64-unknown-unknown` | ? | | WebAssembly |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please consider adding some minimal target documentation for wasm64-unknown-unknown, such as to document the default features it expects in the runtime.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure what you want added here? I mentioned above that I don't think it makes sense to document features and I would rather remove the features than try to document them, but otherwise I don't know how much to document this or what should be included here.
This comment has been minimized.
This comment has been minimized.
a70480a
to
37b970c
Compare
Ok I pushed up a few more commits (the latest two). One works around a crash in LLVM that's wasm64-specific and has an open LLVM bug for it, and the other is some results from running the test suite for wasm64. I've opted to not actually include any compiletest or src/test/ui/* changes just yet since the test suite isn't fully passing, and I don't think it's necessarily useful to have an intermediate state for now. In any case this should be good to go! (this was otherwise just a few other things I noticed today) |
With the latest commit that updates stdarch/compiler-builtins then this PR alone should be enough to make wasm64 a pretty usable target with
That's sort of inevitable for a new target, though, and will require backports and/or custom-built rustc with tip-of-tree LLVM to work around. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
☔ The latest upstream changes (presumably #90392) made this pull request unmergeable. Please resolve the merge conflicts. |
4d46dc9
to
30f60f5
Compare
Rebased! |
ping @yaahc would you be able to take a look at this? If not no worries and I can try to find someone else |
I reached out to @joshtriplett and he said he'd be willing to take over review, so I'm going to... |
|
||
## Target maintainers | ||
|
||
- Alex Crichton, https://github.com/alexcrichton |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I didn't realize this documentation existed before, and I think it's great to have! I also don't mean to usurp a role of sole maintainer here, if others would like to be added I'd be happy to add as part of this PR
This comment has been minimized.
This comment has been minimized.
@bors retry x86_64-gnu-distcheck hung on |
This comment has been minimized.
This comment has been minimized.
⌛ Testing commit af217f7 with merge b192c9da9d53ad26eca875234bfd6140c98ed5e1... |
💔 Test failed - checks-actions |
The job Click to see the possible cause of the failure (guessed by this bot)
|
☀️ Test successful - checks-actions |
Finished benchmarking commit (b6f580a): comparison url. Summary: This change led to large relevant mixed results 🤷 in compiler performance.
If you disagree with this performance assessment, please file an issue in rust-lang/rustc-perf. Next Steps: If you can justify the regressions found in this perf run, please indicate this with @rustbot label: +perf-regression |
Visiting for performance triage. There were a lot of various changes in this PR, such as updates to dependencies ( The flagged regressions of magnitude greater than 0.5% are isolated to "issue-88862 check" and "style-servo check". The improvements tended to outweigh the regressions. For the most part almost all significant performance effects are isolated to check builds. @rustbot label +perf-regression-triaged |
This commit goes through and updates various
#[cfg]
as appropriate toget the wasm64-unknown-unknown target behaving similarly to the
wasm32-unknown-unknown target. Most of this is just updating various
conditions for
target_arch = "wasm32"
to also account fortarget_arch = "wasm64"
where appropriate. This commit also listswasm64
as anallow-listed architecture to not have the
restricted_std
featureenabled, enabling experimentation with
-Z build-std
externally.The main goal of this commit is to enable playing around with
wasm64-unknown-unknown
externally via-Z build-std
in a way that'ssimilar to the
wasm32-unknown-unknown
target. These targets areeffectively the same and only differ in their pointer size, but wasm64
is much newer and has much less ecosystem/library support so it'll still
take time to get wasm64 fully-fledged.