Calling methods on entrypoint argument struct members produces "Pointer operand must be a memory operand declaration" validation error

[Arc-blroth]

Expected Behavior

This code should pass validation, but it doesn't. Interestingly, it seems to also cause a DEVICE_LOST error on my GPU if I ignore validation and run it anyway.
Note that the link is to shader playground, which hasn't updated rust-gpu in a month, but this bug is still present on the latest version.

#![no_std]
#![feature(register_attr)]
#![register_attr(spirv)]

use glam::{Mat4, Vec4};

#[spirv(block)]
#[derive(Clone, Copy)]
pub struct HopesAndDreams {
    pub unicorn: Mat4,
    pub griffin: Mat4,
    pub wyvern:  Mat4,
}

#[allow(unused_attributes)]
#[spirv(vertex)]
pub fn main_vs(hnd: HopesAndDreams, output: &mut Mat4) {
    *output = hnd.unicorn.inverse();
}

Example & Steps To Reproduce

1. Compile the above code.
2. spirv-val (and spirv-opt) complain:

error: invalid id:0:0 - Pointer operand 36[%36] must be a memory object declaration
  %37 = OpFunctionCall %void %glam__f32__mat4__Mat4__inverse %34 %36

  |
  = note: spirv-val failed
  = note: module "D:\\local\\Temp\\bee1e000-03a2-4013-8540-8031be25ccf3\\output\\shader.spv"

3. be sad

System Info

• Rust: rustc 1.53.0-nightly (5d04957a4 2021-03-22)
• OS: Windows 10, Version 2004, Build 19041.867
• GPU: Intel(R) HD Graphics 530
• SPIR-V: SPIRV-Tools v2020.7-dev v2020.6 (but rust-gpu builds its own version of SPIR-V tools, so this is incorrect)

[Arc-blroth]

I've taken a bit more of a look at this, and the issue seems to be a missing OpCopyMemory. The generated ASM is:

%33 = OpVariable %_ptr_Function_HopesAndDreams Function
%34 = OpVariable %_ptr_Function_spirv_std__glam__Mat4 Function
%35 = OpVariable %_ptr_Function_spirv_std__glam__Mat4 Function
      OpCopyMemory %33 %hnd
%36 = OpAccessChain %_ptr_Function_spirv_std__glam__Mat4 %33 %uint_0
%37 = OpFunctionCall %void %glam__f32__mat4__Mat4__inverse %34 %36
      OpCopyMemory %35 %34
      OpCopyMemory %output %35
      OpReturn
      OpFunctionEnd

For reference, GLAM's source code for inverse is

#[inline(always)]
pub fn inverse(&self) -> Self {
    Self(self.0.inverse())
}

It seems that rust-gpu might need to aggressively save entrypoint parameters that are used as function parameters into OpVariables.

[khyperia]

Can you elaborate on why you think it's a missing OpCopyMemory? Looking at that assembly, I don't see any missing OpCopyMemory, so I'd appreciate a pointer on where exactly it should be placed.

[Arc-blroth]

Not exactly sure if this is correct, but I think that the extra instruction would go here:

%33 = OpVariable %_ptr_Function_HopesAndDreams Function
%34 = OpVariable %_ptr_Function_spirv_std__glam__Mat4 Function
%35 = OpVariable %_ptr_Function_spirv_std__glam__Mat4 Function
%36 = OpVariable %_ptr_Function_glam__Vector4x4_XYZW_f32 Function ; type name is probably wrong
      OpCopyMemory %33 %hnd
%37 = OpAccessChain %_ptr_Function_spirv_std__glam__Mat4 %33 %uint_0
      OpCopyMemory %36 %37 ; <-- here
%38 = OpFunctionCall %void %glam__f32__mat4__Mat4__inverse %34 %36
      OpCopyMemory %35 %34
      OpCopyMemory %output %35
      OpReturn
      OpFunctionEnd

From the spec's definition for Memory Object Declaration, it seems that function parameters can generally only be passed with OpVariable's.

[khyperia]

Right, unfortunately that's not a valid fix in general, because the parameter could be &mut, and then any modifications would be modifying the copy instead of the original. A potential other fix is to adjust the inliner to recognize non-memory-object-declarations being passed as arguments and inlining those.

[Arc-blroth]

That's a good point. At least for graphics shaders, it's hard to translate rust that passes around mutable pointers (see below) without inlining everything into a single function, since graphics shaders don't have that concept of pointers.

pub struct Foo {
    pub unicorn: f32,
    pub griffin: f32,
    pub wyvern:  f32,
}

impl Foo {
    fn magic(&mut self) {
      self.unicorn += 1;
    }
}

And since the optimizer already performs fairly aggressive inlining, I think that inlining is probably the simplest solution for these types of situations in general. 👍