add wasm64 support #974

devsnek · 2020-12-30T19:32:32Z

I took the approach of adding a new arch::wasm module which covers both wasm32 and wasm64.

rust-highfive · 2020-12-30T19:32:34Z

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @Amanieu (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

bjorn3 · 2020-12-30T19:45:06Z

crates/core_arch/src/mod.rs

+    #[doc(cfg(any(target_arch = "wasm32", target_arch = "wasm64")))]
+    #[stable(feature = "", since = "0.0.0")]
+    pub mod wasm {
+        #[stable(feature = "", since = "0.0.0")]


This should probably be private. Even x86 and x86_64 are only available on the respective targets despite the fact that x86_64 re-exports everything in x86.

What i was trying to do here was avoid people having to write

#[cfg(target_os = "wasm32")] use core::arch::wasm32 as wasm; #[cfg(target_os = "wasm64")] use core::arch::wasm64 as wasm;

This doesn't match what we do for x86/x86_64 right now, however, where you only get one or the other. I think that we'll want to stick to a wasm32 and a wasm64 module for now to mirror that. The core::arch module isn't optimized for ergonomic usage, but rather optimized for developers (us) to add intrinsics in a known location.

crates/core_arch/src/lib.rs

crates/core_arch/src/wasm/memory.rs

Amanieu · 2020-12-31T16:22:50Z

cc @alexcrichton

alexcrichton · 2021-01-04T15:48:19Z

This looks good to me, thanks! I do think though that there shouldn't be a core::arch::wasm module but instead core::arch::{wasm32, wasm64} modules.

bors · 2021-03-11T23:31:01Z

☔ The latest upstream changes (presumably 8724eb4) made this pull request unmergeable. Please resolve the merge conflicts.

devsnek · 2021-04-05T07:08:43Z

I feel very strongly that we should encourage the abstraction of wasm32 and wasm64, both in rust internals and to users of rust. They are not really different targets, and code that specializes on one of them just makes developing for wasm more difficult.

alexcrichton · 2021-04-05T16:01:19Z

My thinking here is that wasm should follow established precedent primarily and not try to be that different compared to other platforms. It seems like all the issues for wasm32/wasm64 would be the same in Rust's x86 and x86_64 land as well. Given that hasn't been a huge issue leading to trying to have a unified module I'm not sure that it's worth trying to blaze the trail here with wasm.

devsnek · 2021-04-05T16:21:39Z

@alexcrichton what if we also include wasm64 here? so we'd have three. I'm open to other approaches but I'm not interested in landing this with no solution.

joshtriplett · 2021-04-05T17:47:14Z

If we can actually have the same intrinsics on all targets (using usize), then I think it makes sense to have one module, not two.

alexcrichton · 2021-04-05T19:22:54Z

IIRC the x86_64 module reexports everything that has to do with the x86 module, so I would imagine that adding a wasm64 module would do the same thing effectively?

I would definitely want to ensure that, where possible, we use the exact same types across the wasm32/wasm64 modules to enable reexporting. It'd be a bummer if something took an i32 in one and an i64 in the other or something like that.

devsnek · 2021-04-05T19:28:27Z

From my perspective the more important bit is usage of this. I have to go through and make all the core and std code work with both wasm32 and wasm64, which should (imo) be as simple as s/wasm32/wasm/g, but if we don't combine them here i have to add annoying cfgs everywhere to use the correct intrinsic.

alexcrichton · 2021-04-05T19:50:06Z

AFAIK core/std only very lightly use this module, so adding some #[cfg] shouldn't be a monumental task?

bors · 2021-04-07T16:06:05Z

☔ The latest upstream changes (presumably 768b238) made this pull request unmergeable. Please resolve the merge conflicts.

devsnek · 2021-04-29T04:00:48Z

updated to use bad pattern, pls review. also needs something for the #[stable] thingy, not sure what that is.

Amanieu · 2021-04-29T16:18:37Z

crates/core_arch/src/lib.rs

@@ -41,7 +41,7 @@
    bench_black_box
 )]
 #![cfg_attr(test, feature(test, abi_vectorcall))]
-#![cfg_attr(all(test, target_arch = "wasm32"), feature(wasm_simd))]
+#![cfg_attr(all(test, target_arch = "wasm32", target_arch = "wasm64"), feature(wasm_simd))]


Suggested change

#![cfg_attr(all(test, target_arch = "wasm32", target_arch = "wasm64"), feature(wasm_simd))]

#![cfg_attr(all(test, any(target_arch = "wasm32", target_arch = "wasm64")), feature(wasm_simd))]

Otherwise this condition will always be false. But then again tests still seem to pass so maybe it isn't needed?

Amanieu · 2021-04-29T22:53:45Z

crates/core_arch/src/wasm/memory.rs

+    #[link_name = "llvm.wasm.memory.grow"]
+    fn llvm_memory_grow(mem: u32, pages: isize) -> isize;
+    #[link_name = "llvm.wasm.memory.size"]
+    fn llvm_memory_size(mem: u32) -> isize;


Does LLVM still accept these intrinsics without the .i32 at the end? I've noticed that we don't actually have any tests for this.

FWIW the assert_instr on each intrinsic at least asserts that the codegen is correct, but you're right in that I don't think there's any runtime tests for these intrinsics.

Amanieu · 2021-04-29T22:59:55Z

crates/core_arch/src/mod.rs

+    /// See the [module documentation](../index.html) for more details.
+    #[cfg(any(target_arch = "wasm64", doc))]
+    #[doc(cfg(target_arch = "wasm64"))]
+    #[stable(feature = "", since = "0.0.0")]


This should be unstable under simd_wasm64 for now.

Amanieu · 2021-04-29T23:03:09Z

crates/core_arch/src/mod.rs

-    /// generated in your program. This means to generate a binary without SIMD
-    /// you'll need to avoid both options above plus calling into any intrinsics
-    /// in this module.
+    /// See the [module documentation](../index.html) for more details.


This point back to the docs for the arch module, not the wasm module.

Amanieu · 2021-04-29T23:03:29Z

crates/core_arch/src/wasm/mod.rs

+//! enable SIMD via either of these mechanisms, you'll still have SIMD
+//! generated in your program. This means to generate a binary without SIMD
+//! you'll need to avoid both options above plus calling into any intrinsics
+//! in this module.


This module is private so these docs are not actually visible in the documentation.

bors · 2021-05-03T14:58:08Z

☔ The latest upstream changes (presumably c313294) made this pull request unmergeable. Please resolve the merge conflicts.

devsnek · 2021-10-28T19:50:55Z

seems to be replaced by #1240

rust-highfive assigned Amanieu Dec 30, 2020

bjorn3 reviewed Dec 30, 2020

View reviewed changes

sunfishcode reviewed Dec 30, 2020

View reviewed changes

crates/core_arch/src/lib.rs Outdated Show resolved Hide resolved

crates/core_arch/src/wasm/memory.rs Show resolved Hide resolved

devsnek force-pushed the wasm64 branch 3 times, most recently from b025da3 to 1e8e134 Compare April 4, 2021 17:34

devsnek marked this pull request as ready for review April 5, 2021 07:03

Amanieu mentioned this pull request Apr 5, 2021

cfg(wasm) unifying target_arch="wasm32" and target_arch="wasm64" rust-lang/rust#83879

Closed

devsnek force-pushed the wasm64 branch from 1e8e134 to e6bdf1e Compare April 29, 2021 03:44

add wasm64 support

77af60f

devsnek force-pushed the wasm64 branch from e6bdf1e to 77af60f Compare April 29, 2021 03:51

Amanieu reviewed Apr 29, 2021

View reviewed changes

devsnek closed this Oct 28, 2021

devsnek deleted the wasm64 branch January 2, 2023 05:48

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add wasm64 support #974

add wasm64 support #974

devsnek commented Dec 30, 2020 •

edited

Loading

rust-highfive commented Dec 30, 2020

bjorn3 Dec 30, 2020

devsnek Dec 30, 2020

alexcrichton Jan 4, 2021

Amanieu commented Dec 31, 2020

alexcrichton commented Jan 4, 2021

bors commented Mar 11, 2021

devsnek commented Apr 5, 2021

alexcrichton commented Apr 5, 2021

devsnek commented Apr 5, 2021

joshtriplett commented Apr 5, 2021

alexcrichton commented Apr 5, 2021

devsnek commented Apr 5, 2021

alexcrichton commented Apr 5, 2021

bors commented Apr 7, 2021

devsnek commented Apr 29, 2021

Amanieu Apr 29, 2021

Amanieu Apr 29, 2021

alexcrichton Apr 30, 2021

Amanieu Apr 29, 2021

Amanieu Apr 29, 2021

Amanieu Apr 29, 2021

bors commented May 3, 2021

devsnek commented Oct 28, 2021

	#![cfg_attr(all(test, target_arch = "wasm32", target_arch = "wasm64"), feature(wasm_simd))]
	#![cfg_attr(all(test, any(target_arch = "wasm32", target_arch = "wasm64")), feature(wasm_simd))]

add wasm64 support #974

add wasm64 support #974

Conversation

devsnek commented Dec 30, 2020 • edited Loading

rust-highfive commented Dec 30, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Amanieu commented Dec 31, 2020

alexcrichton commented Jan 4, 2021

bors commented Mar 11, 2021

devsnek commented Apr 5, 2021

alexcrichton commented Apr 5, 2021

devsnek commented Apr 5, 2021

joshtriplett commented Apr 5, 2021

alexcrichton commented Apr 5, 2021

devsnek commented Apr 5, 2021

alexcrichton commented Apr 5, 2021

bors commented Apr 7, 2021

devsnek commented Apr 29, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bors commented May 3, 2021

devsnek commented Oct 28, 2021

devsnek commented Dec 30, 2020 •

edited

Loading