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

rustc_target: don't limit SPIR-V inline asm! types to a fixed subset. #79171

Closed
wants to merge 1 commit into from

Conversation

eddyb
Copy link
Member

@eddyb eddyb commented Nov 18, 2020

Follow-up to #78950, which hardcoded I8, I16, I32, I64, F32, F64 as the supported types for SPIR-V asm!.

This PR lifts that limitation and allows any InlineAsmType, as the rust-gpu backend can support any SPIR-V SSA value type.

However, the general type restriction to scalars (integers, floats, pointers) and vectors thereof, remains, as lifting that felt like a more drastic change, but it seems like it might be necessary soon, so if there are no objections, I can also that in this PR.

EDIT: see EmbarkStudios/rust-gpu#256 (comment) for example usecase.

cc @khyperia r? @Amanieu

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

eddyb commented Nov 18, 2020

Also, regarding #78950 (comment), it might be a good idea to switch from reg to id (or something more generic like val).
Though I guess there's no urgency on that, it might still be a good idea to batch all SPIR-V asm! changes together?

@@ -473,6 +471,17 @@ impl fmt::Display for InlineAsmRegOrRegClass {
}
}

/// Type constrains for a particular register class.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
/// Type constrains for a particular register class.
/// Type constraints for a particular register class.

/// Type constrains for a particular register class.
#[derive(Copy, Clone, Debug, Eq, PartialEq)]
pub enum InlineAsmSupportedTypes {
/// Any type is allowed.
Copy link
Member

Choose a reason for hiding this comment

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

Not exactly: this is still limited to primitive types and SIMD vectors.

By the way, is SPIR-V able to handle more exotic types such as i128?

Copy link
Contributor

@khyperia khyperia Nov 19, 2020

Choose a reason for hiding this comment

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

By the way, is SPIR-V able to handle more exotic types such as i128?

For a general background: theoretically yes, integers are declared using an arbitrary bit width (like llvm), and the spec doesn't disallow (or even mention) 128 bit integers. Practically no, though, considering that there are extensions to enable support for 8 bit, 16 bit, and 64 bit integers, which implies no support for 128 bit considering there's no extension for it (the SPIR-V world is full of weird unsaid implications like this).

However, I think in this case, 128 bit integers will either be disallowed outside asm! and so cannot leak into asm! and cause problems, or, they're represented as a pair of u64 or whatever, and so still won't cause asm! problems with unrepresentable types.

Not exactly: this is still limited to primitive types and SIMD vectors.

Yeah, we'd like to allow any type here, not sure if that's easiest to do in this PR or a follow-up (or if it's even possible to allow any type). It's certainly a little weird having an IR in asm!, arbitrary types are just... regular SSA IDs that look exactly like primitive types.

Copy link

Choose a reason for hiding this comment

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

+1 to allowing aggregate Sized types - I've run into this explicitly whilst working on EmbarkStudios/rust-gpu#8 and it's blocking for some of the stuff I'd like to do there. Things get a lot trickier if we need to somehow deconstruct the type into its constituents, and this would also be a pessimisation for the generated code.

None => {
let found_supported_ty = match reg_class.supported_types(asm_arch) {
// FIXME(eddyb) consider skipping the "cannot use value of type" error
// above, letting the codegen backend handle non-scalar/vector types.
Copy link
Member

Choose a reason for hiding this comment

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

We need to do this as part of the type-checking passes since we want to validate these before monomorphization.

Copy link
Member Author

Choose a reason for hiding this comment

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

I meant specifically for the Any case. AFAIK SPIR-V should be able to handle any Sized value (similar to LLVM "first-class aggregrates" but even more first-class).

@JohnCSimon JohnCSimon 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-review Status: Awaiting review from the assignee but also interested parties. labels Dec 7, 2020
@JohnCSimon JohnCSimon 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-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Jan 11, 2021
@JohnCSimon JohnCSimon 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-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Feb 8, 2021
@JohnCSimon JohnCSimon added A-allocators Area: Custom and system allocators 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-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Mar 1, 2021
@Dylan-DPC-zz
Copy link

Closing this as inactive after a discussion with the author

@Dylan-DPC-zz Dylan-DPC-zz added S-inactive Status: Inactive and waiting on the author. This is often applied to closed PRs. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Mar 10, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-allocators Area: Custom and system allocators S-inactive Status: Inactive and waiting on the author. This is often applied to closed PRs.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants