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

Add asm register information for SPIR-V #78950

Merged
merged 2 commits into from
Nov 13, 2020
Merged

Conversation

khyperia
Copy link
Contributor

As discussed in zulip, we at rust-gpu would like to support asm! for our SPIR-V backend. However, we cannot do so purely without frontend support: this match fails and so asm! is not supported (error reported here). 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, 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, 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.

@rust-highfive
Copy link
Collaborator

r? @petrochenkov

(rust_highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Nov 11, 2020
@bjorn3
Copy link
Member

bjorn3 commented Nov 11, 2020

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.

When LLVM adds a SPIR-V target, it will become reachable using a custom target. Before that it will likely already error when trying to start codegen.

@@ -177,6 +179,7 @@ pub enum InlineAsmArch {
Hexagon,
Mips,
Mips64,
Spirv,
Copy link
Member

Choose a reason for hiding this comment

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

Small nit: this should be SpirV.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The formal capitalization is SPIR-V, all caps, so just checking, should it be SPIRV or SpirV? Looks like RISC-V is also all caps and that's spelled RiscV here, so yeah, SpirV?

@petrochenkov
Copy link
Contributor

r? @Amanieu

This matches the capitalization of RiscV
@Amanieu
Copy link
Member

Amanieu commented Nov 12, 2020

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 in the spec), which can be referenced by other instructions. So, reg isn't exactly accurate, they're SSA IDs, not re-assignable registers.

I think reg is fine for now. WASM uses local, so we could also use a custom name here. In the end I don't think it really matters.

@bors r+

@bors
Copy link
Contributor

bors commented Nov 12, 2020

📌 Commit 0e34b73 has been approved by Amanieu

@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 Nov 12, 2020
m-ou-se added a commit to m-ou-se/rust that referenced this pull request Nov 12, 2020
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.
bors added a commit to rust-lang-ci/rust that referenced this pull request Nov 12, 2020
Rollup of 7 pull requests

Successful merges:

 - rust-lang#76730 (Fix rustdoc rendering of by-value mutable arguments in async fn)
 - rust-lang#78836 (Implement destructuring assignment for structs and slices)
 - rust-lang#78857 (Improve BinaryHeap performance)
 - rust-lang#78950 (Add asm register information for SPIR-V)
 - rust-lang#78970 (update rustfmt to v1.4.25)
 - rust-lang#78972 (Update cargo)
 - rust-lang#78987 (extend min_const_generics param ty tests)

Failed merges:

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit 76fa5f2 into rust-lang:master Nov 13, 2020
@rustbot rustbot added this to the 1.49.0 milestone Nov 13, 2020
@khyperia khyperia deleted the spirv-asm branch November 13, 2020 08:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants