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.

Sign up for GitHub

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
Closed

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
25 changes: 15 additions & 10 deletions compiler/rustc_ast_lowering/src/expr.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1396,17 +1396,22 @@ impl<'hir> LoweringContext<'_, 'hir> {
// features. We check that at least one type is available for
// the current target.
let reg_class = reg.reg_class();
for &(_, feature) in reg_class.supported_types(asm_arch) {
if let Some(feature) = feature {
if self.sess.target_features.contains(&Symbol::intern(feature)) {
required_features.clear();
break;
} else {
required_features.push(feature);
match reg_class.supported_types(asm_arch) {
asm::InlineAsmSupportedTypes::Any => {}
asm::InlineAsmSupportedTypes::OneOf(types) => {
for &(_, feature) in types {
if let Some(feature) = feature {
if self.sess.target_features.contains(&Symbol::intern(feature)) {
required_features.clear();
break;
} else {
required_features.push(feature);
}
} else {
required_features.clear();
break;
}
}
} else {
required_features.clear();
break;
}
}
// We are sorting primitive strs here and can use unstable sort here
Expand Down
12 changes: 6 additions & 6 deletions compiler/rustc_codegen_cranelift/src/inline_asm.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,12 +28,12 @@ pub(crate) fn codegen_inline_asm<'tcx>(
let mut outputs = Vec::new();

let mut new_slot = |reg_class: InlineAsmRegClass| {
let reg_size = reg_class
.supported_types(InlineAsmArch::X86_64)
.iter()
.map(|(ty, _)| ty.size())
.max()
.unwrap();
let reg_size = match reg_class.supported_types(InlineAsmArch::X86_64) {
InlineAsmSupportedTypes::OneOf(supported_tys) => {
supported_tys.iter().map(|(ty, _)| ty.size()).max().unwrap()
}
_ => unreachable!(),
};
let align = rustc_target::abi::Align::from_bytes(reg_size.bytes()).unwrap();
slot_size = slot_size.align_to(align);
let offset = slot_size;
Expand Down
18 changes: 13 additions & 5 deletions compiler/rustc_passes/src/intrinsicck.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@ use rustc_middle::ty::{self, Ty, TyCtxt};
use rustc_session::lint;
use rustc_span::{sym, Span, Symbol, DUMMY_SP};
use rustc_target::abi::{Pointer, VariantIdx};
use rustc_target::asm::{InlineAsmRegOrRegClass, InlineAsmType};
use rustc_target::asm::{InlineAsmRegOrRegClass, InlineAsmSupportedTypes, InlineAsmType};
use rustc_target::spec::abi::Abi::RustIntrinsic;

fn check_mod_intrinsics(tcx: TyCtxt<'_>, module_def_id: LocalDefId) {
Expand Down Expand Up @@ -255,10 +255,18 @@ impl ExprVisitor<'tcx> {
// register class.
let asm_arch = self.tcx.sess.asm_arch.unwrap();
let reg_class = reg.reg_class();
let supported_tys = reg_class.supported_types(asm_arch);
let feature = match supported_tys.iter().find(|&&(t, _)| t == asm_ty) {
Some((_, feature)) => feature,
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).

InlineAsmSupportedTypes::Any => Ok((asm_ty, None)),

InlineAsmSupportedTypes::OneOf(supported_tys) => {
supported_tys.iter().find(|&&(t, _)| t == asm_ty).copied().ok_or(supported_tys)
}
};
let feature = match found_supported_ty {
Ok((_, feature)) => feature,
Err(supported_tys) => {
let msg = &format!("type `{}` cannot be used with this register class", ty);
let mut err = self.tcx.sess.struct_span_err(expr.span, msg);
let supported_tys: Vec<_> =
Expand Down
7 changes: 2 additions & 5 deletions compiler/rustc_target/src/asm/aarch64.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
use super::{InlineAsmArch, InlineAsmType};
use super::{InlineAsmArch, InlineAsmSupportedTypes, InlineAsmType};
use rustc_macros::HashStable_Generic;
use std::fmt;

Expand Down Expand Up @@ -50,10 +50,7 @@ impl AArch64InlineAsmRegClass {
}
}

pub fn supported_types(
self,
_arch: InlineAsmArch,
) -> &'static [(InlineAsmType, Option<&'static str>)] {
pub fn supported_types(self, _arch: InlineAsmArch) -> InlineAsmSupportedTypes {
match self {
Self::reg => types! { _: I8, I16, I32, I64, F32, F64; },
Self::vreg | Self::vreg_low16 => types! {
Expand Down
7 changes: 2 additions & 5 deletions compiler/rustc_target/src/asm/arm.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
use super::{InlineAsmArch, InlineAsmType};
use super::{InlineAsmArch, InlineAsmSupportedTypes, InlineAsmType};
use crate::spec::Target;
use rustc_macros::HashStable_Generic;
use std::fmt;
Expand Down Expand Up @@ -42,10 +42,7 @@ impl ArmInlineAsmRegClass {
None
}

pub fn supported_types(
self,
_arch: InlineAsmArch,
) -> &'static [(InlineAsmType, Option<&'static str>)] {
pub fn supported_types(self, _arch: InlineAsmArch) -> InlineAsmSupportedTypes {
match self {
Self::reg | Self::reg_thumb => types! { _: I8, I16, I32, F32; },
Self::sreg | Self::sreg_low16 => types! { "vfp2": I32, F32; },
Expand Down
7 changes: 2 additions & 5 deletions compiler/rustc_target/src/asm/hexagon.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
use super::{InlineAsmArch, InlineAsmType};
use super::{InlineAsmArch, InlineAsmSupportedTypes, InlineAsmType};
use rustc_macros::HashStable_Generic;
use std::fmt;

Expand Down Expand Up @@ -29,10 +29,7 @@ impl HexagonInlineAsmRegClass {
None
}

pub fn supported_types(
self,
_arch: InlineAsmArch,
) -> &'static [(InlineAsmType, Option<&'static str>)] {
pub fn supported_types(self, _arch: InlineAsmArch) -> InlineAsmSupportedTypes {
match self {
Self::reg => types! { _: I8, I16, I32, F32; },
}
Expand Down
7 changes: 2 additions & 5 deletions compiler/rustc_target/src/asm/mips.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
use super::{InlineAsmArch, InlineAsmType};
use super::{InlineAsmArch, InlineAsmSupportedTypes, InlineAsmType};
use rustc_macros::HashStable_Generic;
use std::fmt;

Expand Down Expand Up @@ -30,10 +30,7 @@ impl MipsInlineAsmRegClass {
None
}

pub fn supported_types(
self,
arch: InlineAsmArch,
) -> &'static [(InlineAsmType, Option<&'static str>)] {
pub fn supported_types(self, arch: InlineAsmArch) -> InlineAsmSupportedTypes {
match (self, arch) {
(Self::reg, InlineAsmArch::Mips64) => types! { _: I8, I16, I32, I64, F32, F64; },
(Self::reg, _) => types! { _: I8, I16, I32, F32; },
Expand Down
25 changes: 17 additions & 8 deletions compiler/rustc_target/src/asm/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -137,14 +137,14 @@ macro_rules! types {
) => {
{
use super::InlineAsmType::*;
&[
super::InlineAsmSupportedTypes::OneOf(&[
$($(
($ty, None),
)*)?
$($(
($ty2, Some($feature)),
)*)*
]
])
}
};
}
Expand Down Expand Up @@ -389,12 +389,10 @@ impl InlineAsmRegClass {
}
}

/// Returns a list of supported types for this register class, each with a
/// options target feature required to use this type.
pub fn supported_types(
self,
arch: InlineAsmArch,
) -> &'static [(InlineAsmType, Option<&'static str>)] {
/// Returns either `InlineAsmSupportedTypes::OneOf`, containing a list of
/// supported types for this register class, each with an optional target
/// feature required to use that type, or `InlineAsmSupportedTypes::Any`.
pub fn supported_types(self, arch: InlineAsmArch) -> InlineAsmSupportedTypes {
match self {
Self::X86(r) => r.supported_types(arch),
Self::Arm(r) => r.supported_types(arch),
Expand Down Expand Up @@ -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.

#[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 •
edited
Loading

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.

Any,

/// Only the listed types are supported, and each is paired with
/// an optional target feature required to use that type.
OneOf(&'static [(InlineAsmType, Option<&'static str>)]),
}

/// Set of types which can be used with a particular register class.
#[derive(Copy, Clone, Debug, Eq, PartialEq)]
pub enum InlineAsmType {
Expand Down
7 changes: 2 additions & 5 deletions compiler/rustc_target/src/asm/nvptx.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
use super::{InlineAsmArch, InlineAsmType};
use super::{InlineAsmArch, InlineAsmSupportedTypes, InlineAsmType};
use rustc_macros::HashStable_Generic;

def_reg_class! {
Expand Down Expand Up @@ -30,10 +30,7 @@ impl NvptxInlineAsmRegClass {
None
}

pub fn supported_types(
self,
_arch: InlineAsmArch,
) -> &'static [(InlineAsmType, Option<&'static str>)] {
pub fn supported_types(self, _arch: InlineAsmArch) -> InlineAsmSupportedTypes {
match self {
Self::reg16 => types! { _: I8, I16; },
Self::reg32 => types! { _: I8, I16, I32, F32; },
Expand Down
7 changes: 2 additions & 5 deletions compiler/rustc_target/src/asm/riscv.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
use super::{InlineAsmArch, InlineAsmType};
use super::{InlineAsmArch, InlineAsmSupportedTypes, InlineAsmType};
use crate::spec::Target;
use rustc_macros::HashStable_Generic;
use std::fmt;
Expand Down Expand Up @@ -31,10 +31,7 @@ impl RiscVInlineAsmRegClass {
None
}

pub fn supported_types(
self,
arch: InlineAsmArch,
) -> &'static [(InlineAsmType, Option<&'static str>)] {
pub fn supported_types(self, arch: InlineAsmArch) -> InlineAsmSupportedTypes {
match self {
Self::reg => {
if arch == InlineAsmArch::RiscV64 {
Expand Down
13 changes: 3 additions & 10 deletions compiler/rustc_target/src/asm/spirv.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
use super::{InlineAsmArch, InlineAsmType};
use super::{InlineAsmArch, InlineAsmSupportedTypes, InlineAsmType};
use rustc_macros::HashStable_Generic;

def_reg_class! {
Expand Down Expand Up @@ -28,15 +28,8 @@ impl SpirVInlineAsmRegClass {
None
}

pub fn supported_types(
self,
_arch: InlineAsmArch,
) -> &'static [(InlineAsmType, Option<&'static str>)] {
match self {
Self::reg => {
types! { _: I8, I16, I32, I64, F32, F64; }
}
}
pub fn supported_types(self, _arch: InlineAsmArch) -> InlineAsmSupportedTypes {
InlineAsmSupportedTypes::Any
}
}

Expand Down
7 changes: 2 additions & 5 deletions compiler/rustc_target/src/asm/x86.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
use super::{InlineAsmArch, InlineAsmType};
use super::{InlineAsmArch, InlineAsmSupportedTypes, InlineAsmType};
use crate::spec::Target;
use rustc_macros::HashStable_Generic;
use std::fmt;
Expand Down Expand Up @@ -93,10 +93,7 @@ impl X86InlineAsmRegClass {
}
}

pub fn supported_types(
self,
arch: InlineAsmArch,
) -> &'static [(InlineAsmType, Option<&'static str>)] {
pub fn supported_types(self, arch: InlineAsmArch) -> InlineAsmSupportedTypes {
match self {
Self::reg | Self::reg_abcd => {
if arch == InlineAsmArch::X86_64 {
Expand Down