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.

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

MSL: BDA function parameters should probably be emitted as pointers instead of references #2440

Open
Hugobros3 opened this issue Jan 26, 2025 · 1 comment

Comments

@Hugobros3
Copy link
Contributor

I've ran into a number of issues with MoltenVK having to do with the way it emits pointers in function parameters, as C++ references instead of C-style pointers. For example this snippet:

%my_kernel_1 = OpFunction %void None %31
         %32 = OpFunctionParameter %_ptr_Function__struct_3
          %a = OpFunctionParameter %int
          %b = OpFunctionParameter %_ptr_PhysicalStorageBuffer_int
%my_kernel_2 = OpLabel
         %38 = OpAccessChain %_ptr_Function_uint %32 %uint_0
         %39 = OpLoad %uint %38
         %43 = OpConvertPtrToU %ulong %b
         %44 = OpExtInst %void %40 1 %41 %a %43
               OpStore %38 %39
               OpReturn
               OpFunctionEnd

gets translated as:

static inline __attribute__((always_inline))
void my_kernel(thread _3& _32, int a, device const int& b)
{
    debugPrintfEXT("hi %d 0x%lx\n", a, reinterpret_cast<ulong>(b));
    _32._m0 = _32._m0;
}

This leads to the following bugs:

  • References to void are forbidden but OpTypePointer ... ... %void is legal SPIR-V that SPIRV-Cross can't translate
  • Broken OpPtrToU: That reinterpret_cast above fails because b is a reference and can't decay to a pointer. You'd need to take the address of b
  • Broken ptr arithmetic for the same reason

Here's a godbolt with a few of these problems condensed to a single sample: https://godbolt.org/z/Tanvao3aT

I'm unsure why this choice was made, perhaps it was a restriction in older Metal ?

@Hugobros3
Copy link
Contributor Author

This might be what #2368 was running into, but that's only a hunch

Hugobros3 added a commit to shady-gang/shady that referenced this issue Jan 27, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

1 participant