Add basic support for struct DSTs #504

Hentropy · 2021-03-19T00:17:35Z

Adds support for the following:

#[spirv(compute(threads(32)))]
pub fn shader(
    #[spirv(descriptor_set = 0, binding = 0)] slice: Uniform<Slice<f32>>,
)

pub struct Slice {
    rta: [T],
}

               OpEntryPoint GLCompute %1 "main_cs" %2
               OpExecutionMode %1 LocalSize 32 1 1
               OpMemberDecorate %_struct_3 0 Offset 0
               OpDecorate %2 DescriptorSet 0
               OpDecorate %2 Binding 0
       %void = OpTypeVoid
      %float = OpTypeFloat 32
%_runtimearr_float = OpTypeRuntimeArray %float
  %_struct_3 = OpTypeStruct %_runtimearr_float
%_ptr_Uniform__struct_3 = OpTypePointer Uniform %_struct_3
       %uint = OpTypeInt 32 0
          %9 = OpTypeFunction %void
          %2 = OpVariable %_ptr_Uniform__struct_3 Uniform
          %1 = OpFunction %void None %9
         %10 = OpLabel
         %11 = OpArrayLength %uint %2 0
               OpReturn
               OpFunctionEnd

However, asm doesn't seem to allow &Slice or &[T], and storage class inference has some issues when trying to use asm to implement indexing into the array:

impl<T: Copy> Slice<T> {
    unsafe fn index(&self, idx: usize) -> f32 {
        let mut res = 0.0;

        asm! {
            "%uint = OpTypeInt 32 0",
            "%uint_0 = OpConstant %uint 0",
            "%rta = OpLoad _ {0}",
            "%element = OpAccessChain typeof{2} %rta %uint_0 {1}",
            "OpStore {2} %element",
            in(reg) &self,
            in(reg) idx,
            in(reg) &mut res,
        };
        res
    }
}

produces:

inference conflict: Instance(Instance { generic_id: 4, generic_args: InferVar(7)..InferVar(8) }) vs Var(InferVar(8))
    in %64 = OpAccessChain 58 63 16 35

Trying to do &self.rta or using self with no reference produces:

error: cannot use value of type `*const Slice<T>` for inline assembly
// ...
note: only integers, floats, SIMD vectors, pointers and function pointers can be used as arguments for inline assembly

khyperia · 2021-03-19T08:33:05Z

Wait, we shouldn't need to use asm! to support indexing, this compiles fine with this PR:

// Simple single entrypoint function test.
// build-pass

use spirv_std::storage_class::{Output, Uniform};

#[spirv(fragment)]
pub fn shader(
    #[spirv(descriptor_set = 0, binding = 0)] slice: Uniform<Slice<f32>>,
    mut out: Output<f32>,
) {
    let x = slice.data[5];
    *out = x;
}

pub struct Slice<T> {
    blah: f32,
    data: [T],
}

However, there's issues with the pointercast->GEP conversion code (recover_access_chain_from_offset) that need to be fixed, this surprisingly doesn't work:

// Simple single entrypoint function test.
// build-pass

use spirv_std::storage_class::{Output, Uniform};

#[spirv(fragment)]
pub fn shader(
    #[spirv(descriptor_set = 0, binding = 0)] slice: Uniform<Slice<f32>>,
    mut out: Output<f32>,
) {
    let x = slice.data[5];
    *out = x;
}

pub struct Slice<T> {
    data: [T],
}

which fails with

error: Cannot cast between pointer types
  --> $DIR/hello_world_2.rs:11:13
   |
11 |     let x = slice.data[5];
   |             ^^^^^^^^^^^^^
   |
   = note: from: *struct Slice<f32> { data: [f32] }
   = note: to: *[f32]

so that needs to be fixed

khyperia · 2021-03-19T14:53:07Z

Fixed the issue in PR 510 ^

Hentropy · 2021-03-19T15:05:15Z

Sweet! Thanks 😄 I'll add a couple tests to this.

khyperia · 2021-03-22T08:36:07Z

crates/rustc_codegen_spirv/src/codegen_cx/entry.rs

@@ -36,8 +38,27 @@ impl<'tcx> CodegenCx<'tcx> {
        };
        let fn_hir_id = self.tcx.hir().local_def_id_to_hir_id(local_id);
        let body = self.tcx.hir().body(self.tcx.hir().body_owned_by(fn_hir_id));
+        const EMPTY: ArgAttribute = ArgAttribute::empty();
        for (abi, arg) in fn_abi.args.iter().zip(body.params) {
            if let PassMode::Direct(_) = abi.mode {


nit: this should probably be turned into a match, the reason it's an if let is because there's only one case

khyperia · 2021-03-22T08:43:51Z

crates/rustc_codegen_spirv/src/codegen_cx/entry.rs

+                    arguments.push(u32::MAX);
+                }
+            }
+        }


This seems pretty ad-hoc and confusing to follow (splitting it up into functions would probably help), and has some issues with it. For example, this compiletest passes, but it shouldn't (no way to get the length of the slice):

// build-pass use spirv_std::storage_class::{Output, Uniform}; #[spirv(fragment)] pub fn shader( #[spirv(descriptor_set = 0, binding = 0)] slice: Uniform<&[f32]>, mut out: Output<f32>, ) { let x = slice[5]; *out = x; }

Hentropy · 2021-03-22T18:07:18Z

#443 will simplify this, so I'll wait on that.

Hentropy · 2021-03-23T22:11:19Z

Changed to a much cleaner implementation now that #443 has landed, which addresses both review points.

This only adds support for DSTs that are structs whose last field is a slice.

pub struct MySlice<T>([T]);
pub struct TrailingSlice<T> {
    fields: u32,
    last_field: [T],
}
pub struct NestedSlice<T>(MySlice<T>);

pub fn my_shader(
    // supported
    my_slice: MySlice<f32>,
    trailing_slice: TrailingSlice<f32>,

    // error!
    slice: &[f32],
    nested_slice: &NestedSlice<f32>,
) {/* .. */}

Adding support for nested slices is a lil tricky. OpArrayLength doesn't take a chain of indices so you need to dig into the Adt's fields until you find the one whose last field is TyKind::Slice, collecting the field indices as you go. Then, loop over the field indices excluding the final one and generate OpConstants, pass the constants into OpAccessChain, and pass the result from that into the OpArrayLength with the index of the last field. I can do that in a follow up, but I wanted to keep this basic.

khyperia · 2021-03-24T07:47:57Z

crates/rustc_codegen_spirv/src/codegen_cx/entry.rs

                match entry_fn_arg.layout.ty.kind() {
-                    TyKind::Ref(..) => var,
-
+                    TyKind::Ref(_, ty, _) if ty.is_sized(self.tcx.at(span), self.param_env()) => iter::once(var).chain(None),


Why are you doing chain(None) here (and below)?

Also, could we extract this into a function? 10 levels of indentation is giving me a headache

khyperia · 2021-03-24T07:55:33Z

crates/rustc_codegen_spirv/src/codegen_cx/entry.rs

+                                hir_param.ty_span,
+                                "Trait objects are not supported.",
+                            ),
+                            _ => unreachable!(),


I don't think this is unreachable. For example, here's a compiletest that hits it:

// build-pass #![feature(extern_types)] use spirv_std as _; extern "C" { pub type Blah; } #[spirv(fragment)] pub fn main(#[spirv(uniform)] x: &mut Blah) {}

Hentropy · 2021-03-25T14:53:56Z

@khyperia I addressed your review. Let me know if there's anything else 🙂

Hentropy · 2021-03-29T15:24:20Z

Added ArrayStride decoration to OpTypeRuntimeArray. I am getting a spurious test fail on my machine from a double define of panic_impl and eh_personality due to glam, however that isn't related to any changes I have made.

XAMPPRocky · 2021-03-29T16:02:16Z

Thank you for your PR!

eddyb

I only have a few nits/simplifications, overall I don't mind this having landed.

eddyb · 2021-03-29T15:46:20Z

crates/spirv-builder/src/test/mod.rs

+    let id = module
+        .entry_points
+        .iter()
+        .find(|inst| inst.operands.last().unwrap().unwrap_literal_string() == func)


Isn't it always operands[1] for the name, whereas further operands are interface variables? https://www.khronos.org/registry/spir-v/specs/unified1/SPIRV.html#OpEntryPoint

eddyb · 2021-03-29T19:17:46Z

crates/spirv-builder/src/test/mod.rs

@@ -159,6 +159,33 @@ fn dis_fn(src: &str, func: &str, expect: &str) {
    assert_str_eq(expect, &func.disassemble())
 }

+fn dis_entry_fn(src: &str, func: &str, expect: &str) {


I was confused by this so maybe dis_entry_stub might be easier to get? A comment would also help, but the name made me think the high-level Rust fn, not the generated stub.

eddyb · 2021-03-29T19:18:17Z

crates/spirv-builder/src/test/mod.rs

+    let mut func = module
+        .functions
+        .into_iter()
+        .find(|f| f.def_id().unwrap() == id)
+        .unwrap();
+    // Compact to make IDs more stable
+    compact_ids(&mut func);
+    use rspirv::binary::Disassemble;
+    assert_str_eq(expect, &func.disassemble())


This part could maybe be shared between the two functions (so only the picking the function ID would be different).

eddyb · 2021-03-29T19:31:28Z

crates/rustc_codegen_spirv/src/codegen_cx/entry.rs

+                // DST struct with fields before the DST member
+                | PassMode::Pair(
+                    ArgAttributes { .. },
+                    ArgAttributes {
+                        pointee_size: Size::ZERO,
+                        ..
+                    },


The second element in a pair for &SomeUnsizedType is always the "metadata" like usize for anything based on slices. I assume pointee_size is just zero for non-pointers (and I'm not sure checking pointee_size even makes sense?).

You could probably just check that the type of the PassMode::Pair argument is TyKind::Ref, that should limit it to &SomeUnsizedType.

eddyb · 2021-03-29T19:31:50Z

crates/rustc_codegen_spirv/src/codegen_cx/entry.rs

-        entry_fn_abi: &FnAbi<'tcx, Ty<'tcx>>,
+        arg_abis: &[ArgAbi<'tcx, Ty<'tcx>>],


I'm not sure this is a good idea, especially if we want to do something with the return type at some point.

* Add basic support for struct DSTs * Add tests * cleanup tests * Update with entry changes, address review * Address review * Update allocate_const_scalar.stderr * Add ArrayStride decoration to OpTypeRuntimeArray

Add basic support for struct DSTs

1947777

khyperia mentioned this pull request Mar 19, 2021

Fix recover_access_chain_from_offset on unsized fields #510

Merged

Hentropy added 3 commits March 19, 2021 11:37

Merge branch 'main' into Add-support-for-struct-DSTs

4c06864

Add tests

7e9728d

cleanup tests

57abe13

Hentropy marked this pull request as ready for review March 19, 2021 21:34

Hentropy requested review from eddyb and khyperia as code owners March 19, 2021 21:34

Hentropy mentioned this pull request Mar 21, 2021

Supporting OpRuntimeArray #503

Closed

khyperia suggested changes Mar 22, 2021

View reviewed changes

Hentropy added 2 commits March 23, 2021 11:52

Merge branch 'main' into Add-support-for-struct-DSTs

401fcc3

Update with entry changes, address review

22cc775

khyperia suggested changes Mar 24, 2021

View reviewed changes

Address review

367ac4d

khyperia approved these changes Mar 25, 2021

View reviewed changes

Hentropy added 2 commits March 26, 2021 14:06

Update allocate_const_scalar.stderr

7b9f73f

Add ArrayStride decoration to OpTypeRuntimeArray

2704ef1

Merge branch 'main' into Add-support-for-struct-DSTs

e8df0ac

XAMPPRocky merged commit 05ce407 into EmbarkStudios:main Mar 29, 2021

Hentropy deleted the Add-support-for-struct-DSTs branch March 29, 2021 16:02

eddyb approved these changes Mar 29, 2021

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add basic support for struct DSTs #504

Add basic support for struct DSTs #504

Hentropy commented Mar 19, 2021

khyperia commented Mar 19, 2021

khyperia commented Mar 19, 2021

Hentropy commented Mar 19, 2021

khyperia Mar 22, 2021

khyperia Mar 22, 2021

Hentropy commented Mar 22, 2021

Hentropy commented Mar 23, 2021 •

edited

Loading

khyperia Mar 24, 2021 •

edited

Loading

khyperia Mar 24, 2021

Hentropy commented Mar 25, 2021

Hentropy commented Mar 29, 2021

XAMPPRocky commented Mar 29, 2021

eddyb left a comment

eddyb Mar 29, 2021

eddyb Mar 29, 2021

eddyb Mar 29, 2021

eddyb Mar 29, 2021

eddyb Mar 29, 2021

		entry_fn_abi: &FnAbi<'tcx, Ty<'tcx>>,
		arg_abis: &[ArgAbi<'tcx, Ty<'tcx>>],

Add basic support for struct DSTs #504

Add basic support for struct DSTs #504

Conversation

Hentropy commented Mar 19, 2021

khyperia commented Mar 19, 2021

khyperia commented Mar 19, 2021

Hentropy commented Mar 19, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Hentropy commented Mar 22, 2021

Hentropy commented Mar 23, 2021 • edited Loading

khyperia Mar 24, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Hentropy commented Mar 25, 2021

Hentropy commented Mar 29, 2021

XAMPPRocky commented Mar 29, 2021

eddyb left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Hentropy commented Mar 23, 2021 •

edited

Loading

khyperia Mar 24, 2021 •

edited

Loading