Skip to content
This repository was archived by the owner on Oct 31, 2025. It is now read-only.

Implement OpTypeMatrix #738

Merged
merged 12 commits into from
Aug 30, 2021
Merged

Implement OpTypeMatrix #738

Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
114b2ed
Implement OpTypeMatrix
hatoo Aug 28, 2021
458ddb4
clippy
hatoo Aug 29, 2021
36282ca
Use cached Symbol
hatoo Aug 29, 2021
47b9f92
Implement #[spirv(matrix(ty, m, n))] instead of Matrix trait
hatoo Aug 29, 2021
9dda749
Update #[spirv(matrix(..))]
hatoo Aug 30, 2021
8ec4f2d
Drop #[spirv(matrix(..))] (with arguments)
hatoo Aug 30, 2021
cb88ec2
Fix IntrinsicType::Matrix type construction
hatoo Aug 30, 2021
fa0f62a
Update matrix-type.rs
hatoo Aug 30, 2021
f564761
Update tests/ui/spirv-attr/multiple.rs to test Matrix
hatoo Aug 30, 2021
75af54f
Fix tests/ui/spirv-attr/matrix-type.rs
hatoo Aug 30, 2021
051b3a3
Add failing tests for #[spirv(matrix)
hatoo Aug 30, 2021
572d93e
Update error messages for #[spirv(matrix)]
hatoo Aug 30, 2021
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
38 changes: 38 additions & 0 deletions crates/rustc_codegen_spirv/src/abi.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -843,5 +843,43 @@ fn trans_intrinsic_type<'tcx>(
Err(ErrorReported)
}
}
IntrinsicType::Matrix => {
let (elem_type, n) =
if let Abi::Vector { ref element, count } = ty.field(cx, 0).layout.abi {
match &element.value {
Primitive::F32 => Some(SpirvType::Float(32)),
Primitive::F64 => Some(SpirvType::Float(64)),
Primitive::Int(integer, signed) => {
Some(SpirvType::Integer(integer.size().bits() as u32, *signed))
}
Primitive::Pointer => None,
}
.map(|elem_type| (elem_type, count as u32))
} else {
None
}
.ok_or_else(|| {
cx.tcx
.sess
.err("#[spirv(matrix)] type must have a vector with scalar element type");
ErrorReported
})?;

let m = ty.fields.count() as u32;

let elem_spirv = elem_type.def(span, cx);

let vector = SpirvType::Vector {
element: elem_spirv,
count: n as u32,
}
.def(span, cx);

Ok(SpirvType::Matrix {
element: vector,
count: m as u32,
}
.def(span, cx))
}
Copy link
Contributor

@khyperia khyperia Aug 30, 2021

Choose a reason for hiding this comment

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

You don't have to (and shouldn't) do this digging into field types for this, and should use the standard type translation tools on the fields instead. Also, this is missing quite a bit of validation (e.g. if fields are different types, or there are no fields - IIRC your code ICEs if there's no fields) - there should be tests for each of the three kinds of error. Something like this should work instead:

IntrinsicType::Matrix => {
    let field_types = (0..ty.fields.count())
        .map(|i| trans_type_impl(cx, span, ty.field(cx, i), false))
        .collect::<Vec<_>>();
    if field_types.is_empty() {
        cx.tcx
            .sess
            .err("#[spirv(matrix)] type must have at least one field");
        return Err(ErrorReported);
    }
    let elem_type = field_types[0];
    if !field_types.iter().all(|&ty| ty == elem_type) {
        cx.tcx
            .sess
            .err("#[spirv(matrix)] type fields must all be the same type");
        return Err(ErrorReported);
    }
    match cx.lookup_type(elem_type) {
        SpirvType::Vector { .. } => (),
        ty => {
            cx.tcx
                .sess
                .struct_err("#[spirv(matrix)] type fields must all be vectors")
                .note(&format!("field type is {}", ty.debug(elem_type, cx)))
                .emit();
            return Err(ErrorReported);
        }
    }

    Ok(SpirvType::Matrix {
        element: elem_type,
        count: field_types.len() as u32,
    }
    .def(span, cx))
}

Copy link
Contributor Author

@hatoo hatoo Aug 30, 2021

Choose a reason for hiding this comment

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

Thanks.
I updated the code and change Matrix length validation since OpTypeMatrix requires a length of at least 2.
https://www.khronos.org/registry/SPIR-V/specs/unified1/SPIRV.html#OpTypeMatrix

}
}
1 change: 1 addition & 0 deletions crates/rustc_codegen_spirv/src/attr.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -65,6 +65,7 @@ pub enum IntrinsicType {
SampledImage,
RayQueryKhr,
RuntimeArray,
Matrix,
}

// NOTE(eddyb) when adding new `#[spirv(...)]` attributes, the tests found inside
Expand Down
17 changes: 11 additions & 6 deletions crates/rustc_codegen_spirv/src/builder/builder_methods.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -210,7 +210,7 @@ impl<'a, 'tcx> Builder<'a, 'tcx> {
)),
},
SpirvType::Adt { .. } => self.fatal("memset on structs not implemented yet"),
SpirvType::Vector { element, count } => {
SpirvType::Vector { element, count } | SpirvType::Matrix { element, count } => {
let elem_pat = self.memset_const_pattern(&self.lookup_type(element), fill_byte);
self.constant_composite(
ty.clone().def(self.span(), self),
Expand Down Expand Up @@ -277,7 +277,7 @@ impl<'a, 'tcx> Builder<'a, 'tcx> {
)
.unwrap()
}
SpirvType::Vector { element, count } => {
SpirvType::Vector { element, count } | SpirvType::Matrix { element, count } => {
let elem_pat = self.memset_dynamic_pattern(&self.lookup_type(element), fill_var);
self.emit()
.composite_construct(
Expand Down Expand Up @@ -426,7 +426,8 @@ impl<'a, 'tcx> Builder<'a, 'tcx> {
}
SpirvType::Vector { element, .. }
| SpirvType::Array { element, .. }
| SpirvType::RuntimeArray { element } => {
| SpirvType::RuntimeArray { element }
| SpirvType::Matrix { element, .. } => {
ty = element;
ty_kind = self.lookup_type(ty);

Expand Down Expand Up @@ -1080,7 +1081,8 @@ impl<'a, 'tcx> BuilderMethods<'a, 'tcx> for Builder<'a, 'tcx> {
} => field_types[idx as usize],
SpirvType::Array { element, .. }
| SpirvType::RuntimeArray { element, .. }
| SpirvType::Vector { element, .. } => element,
| SpirvType::Vector { element, .. }
| SpirvType::Matrix { element, .. } => element,
SpirvType::InterfaceBlock { inner_type } => {
assert_eq!(idx, 0);
inner_type
Expand All @@ -1107,7 +1109,8 @@ impl<'a, 'tcx> BuilderMethods<'a, 'tcx> for Builder<'a, 'tcx> {
SpirvType::Adt { field_offsets, .. } => field_offsets[idx as usize],
SpirvType::Array { element, .. }
| SpirvType::RuntimeArray { element, .. }
| SpirvType::Vector { element, .. } => {
| SpirvType::Vector { element, .. }
| SpirvType::Matrix { element, .. } => {
self.lookup_type(element).sizeof(self).unwrap() * idx
}
_ => unreachable!(),
Expand Down Expand Up @@ -1843,7 +1846,9 @@ impl<'a, 'tcx> BuilderMethods<'a, 'tcx> for Builder<'a, 'tcx> {
fn extract_value(&mut self, agg_val: Self::Value, idx: u64) -> Self::Value {
let result_type = match self.lookup_type(agg_val.ty) {
SpirvType::Adt { field_types, .. } => field_types[idx as usize],
SpirvType::Array { element, .. } | SpirvType::Vector { element, .. } => element,
SpirvType::Array { element, .. }
| SpirvType::Vector { element, .. }
| SpirvType::Matrix { element, .. } => element,
other => self.fatal(&format!(
"extract_value not implemented on type {:?}",
other
Expand Down
5 changes: 5 additions & 0 deletions crates/rustc_codegen_spirv/src/builder/spirv_asm.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -303,6 +303,11 @@ impl<'cx, 'tcx> Builder<'cx, 'tcx> {
count: inst.operands[1].unwrap_literal_int32(),
}
.def(self.span(), self),
Op::TypeMatrix => SpirvType::Matrix {
element: inst.operands[0].unwrap_id_ref(),
count: inst.operands[1].unwrap_literal_int32(),
}
.def(self.span(), self),
Op::TypeArray => {
self.err("OpTypeArray in asm! is not supported yet");
return;
Expand Down
15 changes: 15 additions & 0 deletions crates/rustc_codegen_spirv/src/codegen_cx/constant.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -482,6 +482,21 @@ impl<'tcx> CodegenCx<'tcx> {
*offset = final_offset;
result
}
SpirvType::Matrix { element, count } => {
let total_size = ty_concrete
.sizeof(self)
.expect("create_const_alloc: Matrices must be sized");
let final_offset = *offset + total_size;
let values = (0..count).map(|_| {
self.create_const_alloc2(alloc, offset, element)
.def_cx(self)
});
let result = self.constant_composite(ty, values);
assert!(*offset <= final_offset);
// Matrices sometimes have padding at the end (e.g. Mat4x3), skip over it.
*offset = final_offset;
result
}
SpirvType::RuntimeArray { element } => {
let mut values = Vec::new();
while offset.bytes_usize() != alloc.len() {
Expand Down
2 changes: 1 addition & 1 deletion crates/rustc_codegen_spirv/src/codegen_cx/type_.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -174,7 +174,7 @@ impl<'tcx> BaseTypeMethods<'tcx> for CodegenCx<'tcx> {
TypeKind::Struct
}
SpirvType::Vector { .. } => TypeKind::Vector,
SpirvType::Array { .. } | SpirvType::RuntimeArray { .. } => TypeKind::Array,
SpirvType::Array { .. } | SpirvType::RuntimeArray { .. } | SpirvType::Matrix { .. } => TypeKind::Array,
SpirvType::Pointer { .. } => TypeKind::Pointer,
SpirvType::Function { .. } => TypeKind::Function,
// HACK(eddyb) this is probably the closest `TypeKind` (which is still
Expand Down
21 changes: 17 additions & 4 deletions crates/rustc_codegen_spirv/src/spirv_type.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,6 +47,11 @@ pub enum SpirvType {
/// Note: vector count is literal.
count: u32,
},
Matrix {
element: Word,
/// Note: matrix count is literal.
count: u32,
},
Array {
element: Word,
/// Note: array count is ref to constant.
Expand Down Expand Up @@ -174,6 +179,7 @@ impl SpirvType {
result
}
Self::Vector { element, count } => cx.emit_global().type_vector_id(id, element, count),
Self::Matrix { element, count } => cx.emit_global().type_matrix_id(id, element, count),
Self::Array { element, count } => {
// ArrayStride decoration wants in *bytes*
let element_size = cx
Expand Down Expand Up @@ -347,6 +353,7 @@ impl SpirvType {
Self::Vector { element, count } => {
cx.lookup_type(element).sizeof(cx)? * count.next_power_of_two() as u64
}
Self::Matrix { element, count } => cx.lookup_type(element).sizeof(cx)? * count as u64,
Self::Array { element, count } => {
cx.lookup_type(element).sizeof(cx)? * cx.builder.lookup_const_u64(count).unwrap()
}
Expand Down Expand Up @@ -377,9 +384,9 @@ impl SpirvType {
.bytes(),
)
.expect("alignof: Vectors must have power-of-2 size"),
Self::Array { element, .. } | Self::RuntimeArray { element } => {
cx.lookup_type(element).alignof(cx)
}
Self::Array { element, .. }
| Self::RuntimeArray { element }
| Self::Matrix { element, .. } => cx.lookup_type(element).alignof(cx),
Self::Pointer { .. } => cx.tcx.data_layout.pointer_align.abi,
Self::Image { .. }
| Self::AccelerationStructureKhr
Expand Down Expand Up @@ -455,6 +462,12 @@ impl fmt::Debug for SpirvTypePrinter<'_, '_> {
.field("element", &self.cx.debug_type(element))
.field("count", &count)
.finish(),
SpirvType::Matrix { element, count } => f
.debug_struct("Matrix")
.field("id", &self.id)
.field("element", &self.cx.debug_type(element))
.field("count", &count)
.finish(),
SpirvType::Array { element, count } => f
.debug_struct("Array")
.field("id", &self.id)
Expand Down Expand Up @@ -612,7 +625,7 @@ impl SpirvTypePrinter<'_, '_> {
}
f.write_str(" }")
}
SpirvType::Vector { element, count } => {
SpirvType::Vector { element, count } | SpirvType::Matrix { element, count } => {
ty(self.cx, stack, f, element)?;
write!(f, "x{}", count)
}
Expand Down
4 changes: 4 additions & 0 deletions crates/rustc_codegen_spirv/src/symbols.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -334,6 +334,10 @@ impl Symbols {
"runtime_array",
SpirvAttribute::IntrinsicType(IntrinsicType::RuntimeArray),
),
(
"matrix",
SpirvAttribute::IntrinsicType(IntrinsicType::Matrix),
),
("unroll_loops", SpirvAttribute::UnrollLoops),
]
.iter()
Expand Down
43 changes: 43 additions & 0 deletions tests/ui/spirv-attr/matrix-type.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,43 @@
// build-pass
// compile-flags: -Ctarget-feature=+RayTracingKHR,+ext:SPV_KHR_ray_tracing

use spirv_std as _;

#[derive(Clone, Copy)]
#[spirv(matrix)]
pub struct Affine3 {
pub x: glam::Vec3,
pub y: glam::Vec3,
pub z: glam::Vec3,
pub w: glam::Vec3,
}

impl Affine3 {
pub const ZERO: Self = Self {
x: glam::Vec3::ZERO,
y: glam::Vec3::ZERO,
z: glam::Vec3::ZERO,
w: glam::Vec3::ZERO,
};

pub const IDENTITY: Self = Self {
x: glam::Vec3::X,
y: glam::Vec3::Y,
z: glam::Vec3::Z,
w: glam::Vec3::ZERO,
};
}

impl Default for Affine3 {
#[inline]
fn default() -> Self {
Self::IDENTITY
}
}

#[spirv(closest_hit)]
pub fn main(
#[spirv(object_to_world)] _object_to_world: Affine3,
#[spirv(world_to_object)] _world_to_object: Affine3,
) {
}
Copy link
Contributor

@khyperia khyperia Aug 30, 2021

Choose a reason for hiding this comment

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

I believe this isn't actually testing much due to the majority of it being dead code. (Also, nit, the name Affine3 is a little misleading - it's not an affine transformation - but isn't super important for a test, haha)

I would like to see field accesses/etc. tested as well, I'm nervous about just crossing our fingers and hoping that matricies behave exactly like structs in all ways and no instructions need to be modified to handle matricies specially.

Copy link
Contributor Author

@hatoo hatoo Aug 30, 2021

Choose a reason for hiding this comment

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

Oops, I chose Affine3 because glam::f32::Affine3A has the same layout https://docs.rs/glam/0.18.0/glam/f32/struct.Affine3A.html (Sry, I don't know about math 😨).

I feel adding more tests for field operations in tests/ui/spirv-attr/* is not appropriate because it may should contain only about attr tests.
Is it OK? or is there any better place?

Copy link
Contributor

@khyperia khyperia Aug 30, 2021

Choose a reason for hiding this comment

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

Oh! whoops, misread it as 4 Vec4s, not 4 Vec3s, 4 Vec3s definitely is an affine transform, haha, sorry

Yeah, not totally sure about where to put tests, anywhere is probably fine

Copy link
Contributor Author

@hatoo hatoo Aug 30, 2021

Choose a reason for hiding this comment

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

I added tests to matrix-type.rs I think this is enough but are there suggestions for more cases?