Implement OpTypeMatrix #738

hatoo · 2021-08-29T07:11:05Z

Fixes #82.

This PR adds #[spirv(matrix(ty, m, n))] attribute. A type with #[spirv(matrix(..))] is expressed as OpTypeMatrix in SPIR-V.
An example usage is tests/ui/spirv-attr/matrix-type.rs.

~~While OpTypeVector is implemented by utilizing #[repr(simd)], this PR implements OpTypeMatrix by introducing the new trait Matrix because I couldn't find a usable attribute.~~

~~When converting Ty to SPIRV type, it uses OpTypeMatrix if the Ty implements Matrix.~~

TODO

Make glam matrices OpMatrix
Since many operations in glam's matrices cast self to inner type, we need to add attributes to glam matrices and its inner types.
``Currently, we can't add the attribute to glam::core::storage::Column*<V> because we can't know its vector length at definition.
We may handle it by adding `#[spirv(matrix(m, param(n)))]` syntax and refer nth generics type (e.g. glam::Vec3) and infer vector length from it...

hatoo · 2021-08-29T13:32:22Z

I've realized that we can implement this with custom attributes and I think it's better.
I've removed Matrix trait and added #[spirv(matrix(..))].
I'll revert it if you prefer the Matrix trait.

hatoo · 2021-08-30T05:38:24Z

I added

#[spirv(matrix(ty, m))]
#[spirv(matrix(ty))]
#[spirv(matrix)]

These infer missing parameters from type.

- #[spirv(matrix(ty, m, n))] Specify all of type, rows, columns. - #[spirv(matrix(ty, m))] Specify all of type, rows. Infer columns. - #[spirv(matrix(ty))] Specify all of type. Infer others. - #[spirv(matrix)] Infer all.

khyperia · 2021-08-30T07:00:06Z

Please only implement #[spirv(matrix)] (the inferred one), it's both the preferential design and simplifies a lot of stuff.

hatoo · 2021-08-30T07:30:55Z

Make sense!
I dropped other than #[spirv(matrix)]

khyperia

I believe tests/ui/spirv-attr/{invalid-target.rs,multiple.rs} need to be updated.

khyperia · 2021-08-30T08:02:19Z

crates/rustc_codegen_spirv/src/abi.rs

+                count: m as u32,
+            }
+            .def(span, cx))
+        }


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)) }

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

khyperia · 2021-08-30T08:08:40Z

tests/ui/spirv-attr/matrix-type.rs

+    #[spirv(object_to_world)] _object_to_world: Affine3,
+    #[spirv(world_to_object)] _world_to_object: Affine3,
+) {
+}


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.

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?

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

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

hatoo · 2021-08-30T09:08:55Z

Thanks for pointed out.
I updated tests/ui/spirv-attr/multiple.rs but failed to update tests/ui/spirv-attr/invalid-target.rs...

I simply replaced sampler, block, sampled_image, generic_image_type, // struct-only to sampler, block, sampled_image, generic_image_type, matrix, // struct-only.

But got the error

error: failed to decode compiler output as json: `EOF while parsing a string at line 1 column 239`

khyperia · 2021-08-30T09:35:52Z

I simply replaced sampler, block, sampled_image, generic_image_type, // struct-only to sampler, block, sampled_image, generic_image_type, matrix, // struct-only.

But got the error
error: failed to decode compiler output as json: `EOF while parsing a string at line 1 column 239`

Looks like compiletest is just straight up broken if output is too long - it injects this into the output and then later tries to parse the output as json, tripping over its own trimmed marker.

Feel free to just skip updating invalid-target.rs for now. (or, split it into multiple files if you feel like it so the output is shorter, but, I can do that after this PR is merged)

hatoo · 2021-08-30T10:18:20Z

I can't reproduce the CI failure.
I've tested on Windows 11 and Ubuntu20.04 (both WSL and native) and pass.

khyperia · 2021-08-30T10:26:34Z

I'm also unable to reproduce, even with CI's exact version of spirv-tools. hmm. I'll try rerunning tests, I guess?

edit: oh, I mucked up getting CI's exact version of spirv-tools, I can reproduce locally now. Assuming linux, download/extract this tarball, put the folder on PATH, then run tests via cargo run -p compiletests --release --no-default-features --features "use-installed-tools" -- --target-env vulkan1.1,spv1.3

khyperia · 2021-08-30T10:51:27Z

Was able to reproduce locally, the full error was cut off in CI:

error: line 70: OpEntryPoint Entry Point <id> '4[%4]'s callgraph contains function <id> 4[%4], which cannot be used with the current execution model:
[VUID-StandaloneSpirv-None-04644] in Vulkan evironment, Output Storage Class must not be used in GLCompute, RayGenerationKHR, IntersectionKHR, AnyHitKHR, ClosestHitKHR, MissKHR, or CallableKHR execution models

  %4 = OpFunction %void None %28

the issue seems to be an output (&mut) var being used in closest_hit functions

hatoo · 2021-08-30T10:56:03Z

Thank you!
I updated test cases.

khyperia · 2021-08-30T11:12:57Z

could you add a // build-fail test that triggers the three new errors? (also remove the accidentally commited module file) - other than that, looks pretty good!

hatoo · 2021-08-30T11:37:23Z

I don't still understand why

#[spirv(fragment)]
pub fn main_add(in1: Affine3, in2: Affine3, out: &mut glam::Vec3) {
    *out = in1.x + in1.y + in1.z + in1.w + in2.x + in2.y + in2.z + in2.w;
}

failed but reverted to previous code for now.

khyperia · 2021-08-30T11:44:30Z

Ah, I think that failure is due to a similar thing as #513, which, might become more pressing to fix once people actually start using matrices. TL;DR matricies take up a handful of binding slots, so incrementing binding slot by 1 per parameter doesn't work (and results in overlapping parameter slots)

hatoo · 2021-08-30T11:56:31Z

I added failing tests.

I added two test files because it seems not able to test #[(spirv(matrix))] for empty struct with others in the same file.

Error messages feel unkind because it doesn't show type definition.
But we can't get a span of type definition in trans_intrinsic_type, right?

khyperia · 2021-08-30T12:00:37Z

Oh, oof, no, that's why we add tests, haha, I completely screwed up that prototype impl - there is a span in scope (it's passed into trans_intrinsic_type, should be able to use span_err instead of err (and struct_span_err instead of struct_err)

khyperia · 2021-08-30T12:04:38Z

Ah, but you're right that we probably want to point the error at the struct itself, in which case, this gives you the span for the struct - you might have to re-match on the TyKind::Adt to pull it out again, I forget if there's a helper that makes it easier

rust-gpu/crates/rustc_codegen_spirv/src/abi.rs

Line 327 in 1e3881b

span = cx.tcx.def_span(adt.did);

hatoo · 2021-08-30T12:29:26Z

Thanks, I choose to use def_id_for_spirv_type_adt and it will returns never None because of trans_type_impl.

khyperia

🎉

hatoo · 2021-08-30T13:51:23Z

Woo! 🎉 🎉 🎉
I really appreciate your support and thank you for your time! 🙏

Implement OpTypeMatrix

114b2ed

hatoo requested review from eddyb and khyperia as code owners August 29, 2021 07:11

hatoo mentioned this pull request Aug 29, 2021

Support #[spirv(object_to_world)] and #[spirv(world_to_object)] #734

Closed

hatoo added 2 commits August 29, 2021 16:24

clippy

458ddb4

Use cached Symbol

36282ca

Implement #[spirv(matrix(ty, m, n))] instead of Matrix trait

47b9f92

hatoo force-pushed the op-type-matrix branch from 713d0af to 47b9f92 Compare August 29, 2021 13:35

hatoo force-pushed the op-type-matrix branch from 14f3115 to dc3a1e1 Compare August 30, 2021 05:45

Update #[spirv(matrix(..))]

9dda749

- #[spirv(matrix(ty, m, n))] Specify all of type, rows, columns. - #[spirv(matrix(ty, m))] Specify all of type, rows. Infer columns. - #[spirv(matrix(ty))] Specify all of type. Infer others. - #[spirv(matrix)] Infer all.

hatoo force-pushed the op-type-matrix branch from dc3a1e1 to 9dda749 Compare August 30, 2021 06:01

Drop #[spirv(matrix(..))] (with arguments)

8ec4f2d

khyperia suggested changes Aug 30, 2021

View reviewed changes

hatoo added 3 commits August 30, 2021 17:15

Fix IntrinsicType::Matrix type construction

cb88ec2

Update matrix-type.rs

fa0f62a

Update tests/ui/spirv-attr/multiple.rs to test Matrix

f564761

hatoo force-pushed the op-type-matrix branch from cf9d17e to d4686f3 Compare August 30, 2021 11:17

Fix tests/ui/spirv-attr/matrix-type.rs

75af54f

hatoo force-pushed the op-type-matrix branch from d4686f3 to 75af54f Compare August 30, 2021 11:35

Add failing tests for #[spirv(matrix)

051b3a3

Update error messages for #[spirv(matrix)]

572d93e

khyperia approved these changes Aug 30, 2021

View reviewed changes

khyperia merged commit b0676cb into EmbarkStudios:main Aug 30, 2021

hatoo deleted the op-type-matrix branch August 30, 2021 13:51

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement OpTypeMatrix #738

Implement OpTypeMatrix #738

hatoo commented Aug 29, 2021 •

edited

Loading

hatoo commented Aug 29, 2021 •

edited

Loading

hatoo commented Aug 30, 2021

khyperia commented Aug 30, 2021

hatoo commented Aug 30, 2021

khyperia left a comment

khyperia Aug 30, 2021

hatoo Aug 30, 2021

khyperia Aug 30, 2021

hatoo Aug 30, 2021

khyperia Aug 30, 2021

hatoo Aug 30, 2021

hatoo commented Aug 30, 2021 •

edited

Loading

khyperia commented Aug 30, 2021

hatoo commented Aug 30, 2021 •

edited

Loading

khyperia commented Aug 30, 2021 •

edited

Loading

khyperia commented Aug 30, 2021

hatoo commented Aug 30, 2021

khyperia commented Aug 30, 2021

hatoo commented Aug 30, 2021

khyperia commented Aug 30, 2021 •

edited

Loading

hatoo commented Aug 30, 2021 •

edited

Loading

khyperia commented Aug 30, 2021

khyperia commented Aug 30, 2021

hatoo commented Aug 30, 2021

khyperia left a comment

hatoo commented Aug 30, 2021

Implement OpTypeMatrix #738

Implement OpTypeMatrix #738

Conversation

hatoo commented Aug 29, 2021 • edited Loading

TODO

hatoo commented Aug 29, 2021 • edited Loading

hatoo commented Aug 30, 2021

khyperia commented Aug 30, 2021

hatoo commented Aug 30, 2021

khyperia left a comment

Choose a reason for hiding this comment

khyperia Aug 30, 2021

Choose a reason for hiding this comment

hatoo Aug 30, 2021

Choose a reason for hiding this comment

khyperia Aug 30, 2021

Choose a reason for hiding this comment

hatoo Aug 30, 2021

Choose a reason for hiding this comment

khyperia Aug 30, 2021

Choose a reason for hiding this comment

hatoo Aug 30, 2021

Choose a reason for hiding this comment

hatoo commented Aug 30, 2021 • edited Loading

khyperia commented Aug 30, 2021

hatoo commented Aug 30, 2021 • edited Loading

khyperia commented Aug 30, 2021 • edited Loading

khyperia commented Aug 30, 2021

hatoo commented Aug 30, 2021

khyperia commented Aug 30, 2021

hatoo commented Aug 30, 2021

khyperia commented Aug 30, 2021 • edited Loading

hatoo commented Aug 30, 2021 • edited Loading

khyperia commented Aug 30, 2021

khyperia commented Aug 30, 2021

hatoo commented Aug 30, 2021

khyperia left a comment

Choose a reason for hiding this comment

hatoo commented Aug 30, 2021

hatoo commented Aug 29, 2021 •

edited

Loading

hatoo commented Aug 29, 2021 •

edited

Loading

hatoo commented Aug 30, 2021 •

edited

Loading

hatoo commented Aug 30, 2021 •

edited

Loading

khyperia commented Aug 30, 2021 •

edited

Loading

khyperia commented Aug 30, 2021 •

edited

Loading

hatoo commented Aug 30, 2021 •

edited

Loading