Add wasm32 support to inline asm by devsnek · Pull Request #78684 · rust-lang/rust

devsnek · 2020-11-02T21:32:38Z

There is some contention around inline asm and wasm, and I really only made this to figure out the process of hacking on rustc, but I figured as long as the code existed, it was worth uploading.

cc @Amanieu

rust-highfive · 2020-11-02T21:32:41Z

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @davidtwco (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.

Amanieu · 2020-11-03T15:48:25Z

I don't think that inline assembly really makes sense for wasm since you can already do everything that is possible with normal Rust code (possibly using the wasm intrinsics in std::arch). On "real" architectures you can do all sorts of tricks with inline assembly such as code patching, inline syscalls and playing with signal handlers that just don't apply to wasm.

devsnek · 2020-11-03T16:06:48Z

Well, I don't disagree with you, but I still think it is interesting to support.

Amanieu · 2020-11-04T17:24:00Z

You need to add a test in src/test/assembly/asm which tests that LLVM accepts all of the types that are supported by local.

davidtwco

Implementation looks good to me but I'm not that familiar with this code, and can't speak to whether or not this is something we want.

davidtwco · 2020-11-08T12:10:53Z

r? @Amanieu

Add asm register information for SPIR-V As discussed in [zulip](https://rust-lang.zulipchat.com/#narrow/stream/182449-t-compiler.2Fhelp/topic/Defining.20asm!.20for.20new.20architecture), we at [rust-gpu](https://github.com/EmbarkStudios/rust-gpu) would like to support `asm!` for our SPIR-V backend. However, we cannot do so purely without frontend support: [this match](https://github.com/rust-lang/rust/blob/d4ea0b3e46a0303d5802b632e88ba1ba84d9d16f/compiler/rustc_target/src/asm/mod.rs#L185) fails and so `asm!` is not supported ([error reported here](https://github.com/rust-lang/rust/blob/d4ea0b3e46a0303d5802b632e88ba1ba84d9d16f/compiler/rustc_ast_lowering/src/expr.rs#L1095)). To resolve this, we need to stub out register information for SPIR-V to support getting the `asm!` content all the way to [`AsmBuilderMethods::codegen_inline_asm`](https://doc.rust-lang.org/nightly/nightly-rustc/rustc_codegen_ssa/traits/trait.AsmBuilderMethods.html#tymethod.codegen_inline_asm), at which point the rust-gpu backend can do all the parsing and codegen that is needed. This is a pretty weird PR - adding support for a backend that isn't in-tree feels pretty gross to me, but I don't see an easy way around this. `@Amanieu` said I should submit it anyway, so, here we are! Let me know if this needs to go through a more formal process (MCP?) and what I should do to help this along. I based this off the [wasm asm PR](rust-lang#78684), which unfortunately this PR conflicts with that one quite a bit, sorry for any merge conflict pain :( --- Some open questions: - What do we call the register class? Some context, SPIR-V is an SSA-based IR, there are "instructions" that create IDs (referred to as `<id>` in the spec), which can be referenced by other instructions. So, `reg` isn't exactly accurate, they're SSA IDs, not re-assignable registers. - What happens when a SPIR-V register gets to the LLVM backend? Right now it's a `bug!`, but should that be a `sess.fatal()`? I'm not sure if it's even possible to reach that point, maybe there's a check that prevents the `spirv` target from even reaching that codepath.

Add asm register information for SPIR-V As discussed in [zulip](https://rust-lang.zulipchat.com/#narrow/stream/182449-t-compiler.2Fhelp/topic/Defining.20asm!.20for.20new.20architecture), we at [rust-gpu](https://github.com/EmbarkStudios/rust-gpu) would like to support `asm!` for our SPIR-V backend. However, we cannot do so purely without frontend support: [this match](https://github.com/rust-lang/rust/blob/d4ea0b3e46a0303d5802b632e88ba1ba84d9d16f/compiler/rustc_target/src/asm/mod.rs#L185) fails and so `asm!` is not supported ([error reported here](https://github.com/rust-lang/rust/blob/d4ea0b3e46a0303d5802b632e88ba1ba84d9d16f/compiler/rustc_ast_lowering/src/expr.rs#L1095)). To resolve this, we need to stub out register information for SPIR-V to support getting the `asm!` content all the way to [`AsmBuilderMethods::codegen_inline_asm`](https://doc.rust-lang.org/nightly/nightly-rustc/rustc_codegen_ssa/traits/trait.AsmBuilderMethods.html#tymethod.codegen_inline_asm), at which point the rust-gpu backend can do all the parsing and codegen that is needed. This is a pretty weird PR - adding support for a backend that isn't in-tree feels pretty gross to me, but I don't see an easy way around this. ``@Amanieu`` said I should submit it anyway, so, here we are! Let me know if this needs to go through a more formal process (MCP?) and what I should do to help this along. I based this off the [wasm asm PR](rust-lang#78684), which unfortunately this PR conflicts with that one quite a bit, sorry for any merge conflict pain :( --- Some open questions: - What do we call the register class? Some context, SPIR-V is an SSA-based IR, there are "instructions" that create IDs (referred to as `<id>` in the spec), which can be referenced by other instructions. So, `reg` isn't exactly accurate, they're SSA IDs, not re-assignable registers. - What happens when a SPIR-V register gets to the LLVM backend? Right now it's a `bug!`, but should that be a `sess.fatal()`? I'm not sure if it's even possible to reach that point, maybe there's a check that prevents the `spirv` target from even reaching that codepath.

devsnek · 2020-12-01T02:49:34Z

@Amanieu ping

Amanieu · 2020-12-01T09:04:46Z

@bors r+

bors · 2020-12-01T09:04:48Z

📌 Commit 031255abc8c32e42624f5be7d27f86c6caffefe0 has been approved by Amanieu

bors · 2020-12-01T09:29:45Z

⌛ Testing commit 031255abc8c32e42624f5be7d27f86c6caffefe0 with merge 07788d3bf251a30514ddb988146bbbf53b4cf59c...

bors · 2020-12-01T09:58:53Z

💔 Test failed - checks-actions

jyn514 · 2020-12-01T15:31:22Z

failures:
Some tests failed in compiletest suite=assembly mode=assembly host=x86_64-unknown-linux-gnu target=x86_64-unknown-linux-gnu
---- [assembly] assembly/asm/wasm-types.rs stdout ----
error: verification with 'FileCheck' failed
status: exit code: 1
command: "/checkout/obj/build/x86_64-unknown-linux-gnu/llvm/build/bin/FileCheck" "--input-file" "/checkout/obj/build/x86_64-unknown-linux-gnu/test/assembly/asm/wasm-types/wasm-types.s" "/checkout/src/test/assembly/asm/wasm-types.rs"
stdout:
------------------------------------------
------------------------------------------
stderr:
------------------------------------------
/checkout/src/test/assembly/asm/wasm-types.rs:145:11: error: CHECK: expected string not found in input
// CHECK: .set i32_ptr, i32_i32
^
/checkout/obj/build/x86_64-unknown-linux-gnu/test/assembly/asm/wasm-types/wasm-types.s:447:24: note: scanning from here
.section .text.i32_ptr,"",@
^
/checkout/obj/build/x86_64-unknown-linux-gnu/test/assembly/asm/wasm-types/wasm-types.s:451:7: note: possible intended match here
.functype i32_ptr (i32) -> (i32)
^
Input file: /checkout/obj/build/x86_64-unknown-linux-gnu/test/assembly/asm/wasm-types/wasm-types.s
Check file: /checkout/src/test/assembly/asm/wasm-types.rs

Maybe the test needs to be marked wasm-only or something?

devsnek · 2020-12-01T18:20:51Z

it seems like llvm was deduplicating i32_ptr (.set i32_ptr, i32_i32) when i tested it locally, but not when tested on the ci here. i switched out the instruction used in the i32_ptr test so hopefully it passes on ci as well now.

jyn514 · 2020-12-01T19:05:24Z

@bors r=Amanieu rollup=iffy

cc @rust-lang/infra - it's been 45 minutes since the new commits were pushed, but the PR checks aren't running, are we finally out of capacity?

bors · 2020-12-01T19:05:26Z

📌 Commit d9f237c has been approved by Amanieu

emilyalbini · 2020-12-01T19:44:34Z

@jyn514 https://www.githubstatus.com/incidents/sxthxvdlljk4

bors · 2020-12-01T20:23:11Z

⌛ Testing commit d9f237c with merge 6645da3...

bors · 2020-12-01T23:44:16Z

☀️ Test successful - checks-actions
Approved by: Amanieu
Pushing 6645da3 to master...

rust-highfive assigned davidtwco Nov 2, 2020

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Nov 2, 2020

devsnek force-pushed the inline-asm-wasm branch 2 times, most recently from c868459 to bb00fd8Compare November 2, 2020 22:16

jyn514 added A-inline-assembly Area: Inline assembly (`asm!(…)`) O-wasm Target: WASM (WebAssembly), http://webassembly.org/ T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Nov 2, 2020

jyn514 reviewed Nov 2, 2020
View reviewed changes

Comment threadcompiler/rustc_target/src/asm/mod.rs Outdated
Comment threadsrc/test/ui/asm/bad-arch.rs Outdated

davidtwco reviewed Nov 8, 2020
View reviewed changes

rust-highfive assigned Amanieu and unassigned davidtwco Nov 8, 2020

khyperia mentioned this pull request Nov 11, 2020
Add asm register information for SPIR-V #78950
Merged

This comment has been minimized.
Sign in to view

devsnek mentioned this pull request Nov 19, 2020
[WebAssembly] Support fp reg class in r constraint rust-lang/llvm-project#83
Merged

devsnek force-pushed the inline-asm-wasm branch 5 times, most recently from 6ac8cba to 7d1c940Compare November 19, 2020 04:27

update llvm submodule
8a92938

devsnek force-pushed the inline-asm-wasm branch from 7d1c940 to 031255aCompare November 19, 2020 04:28

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 Dec 1, 2020

bors added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Dec 1, 2020

Add wasm32 support to inline asm
d9f237c

devsnek force-pushed the inline-asm-wasm branch from 031255a to d9f237cCompare December 1, 2020 18:18

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 Dec 1, 2020

bors added the merged-by-bors This PR was explicitly merged by bors. label Dec 1, 2020

bors merged commit 6645da3 into rust-lang:masterDec 1, 2020

rustbot added this to the 1.50.0 milestone Dec 1, 2020

devsnek deleted the inline-asm-wasm branch December 2, 2020 00:14

jieyouxu mentioned this pull request Dec 18, 2024
tests/assembly/asm: Remove uses of rustc_attrs and lang_items features by using minicore #134436
Merged

Uh oh!

Uh oh!

Conversation

devsnek commented Nov 2, 2020

Uh oh!

rust-highfive commented Nov 2, 2020

Uh oh!

Uh oh!

Uh oh!

Amanieu commented Nov 3, 2020

Uh oh!

devsnek commented Nov 3, 2020

Uh oh!

Amanieu commented Nov 4, 2020

Uh oh!

davidtwco left a comment

Choose a reason for hiding this comment

Uh oh!

davidtwco commented Nov 8, 2020

Uh oh!

This comment has been minimized.

This comment has been minimized.

devsnek commented Dec 1, 2020

Uh oh!

Amanieu commented Dec 1, 2020

Uh oh!

bors commented Dec 1, 2020

Uh oh!

bors commented Dec 1, 2020

Uh oh!

bors commented Dec 1, 2020

Uh oh!

jyn514 commented Dec 1, 2020

Uh oh!

devsnek commented Dec 1, 2020

Uh oh!

jyn514 commented Dec 1, 2020

Uh oh!

bors commented Dec 1, 2020

Uh oh!

emilyalbini commented Dec 1, 2020

Uh oh!

bors commented Dec 1, 2020

Uh oh!

bors commented Dec 1, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants