shader-slang / slang

Making it easier to work with shaders
MIT License
1.79k stars 160 forks source link

Reinvestigate support for OpDebugTypePointer #4304

Open csyonghe opened 1 month ago

csyonghe commented 1 month ago

SPIRV recently adds an extension to relax restrictions for forward declarations in ExtInst, this should allow us to define forward pointer types in debug insts.

We should reinvestigate if this is now allowed, and report back remaining blockers for us to generate debug info for general pointer types.

https://github.com/KhronosGroup/SPIRV-Headers/pull/434 https://github.com/KhronosGroup/SPIRV-Tools/pull/5698

jkwak-work commented 1 week ago

It appears that the implementation for the forward declaration hasn't been released.

On Vulkan-Header side, the commit, "ff2afc3", was submitted on May-30-2024. And the latest release was on Mar-27-2024, which was "vulkan-sdk-1.3.283".

On Vulkan-Tool side, the commit, "6a2bdee", was submitted on Jun-4-2024. And the latest release was on Apr-22-2024, which was "vulkan-sdk-1.3.283".

I was able to get ToT and build them locally, which unblocks me for the task. But I wonder if this is little too early to make changes based on a feature not released on vulkan-sdk side.

csyonghe commented 1 week ago

We can consider it released as long as the feature is available from tot.

jkwak-work commented 1 week ago

I am having some trouble using the new keyword, "OpExtInstWithForwardRefs". I updated three external directories and still not emitting the new keyword as I expected: external/spirv-headers, external/spirv-tools and external/spirv-tools-generated.

In order to understand it better, I was looking at the implementation on the Spirv-tool. And I found that the testing code for Spirv-Tool uses "OpExtInstWithForwardRefs" only with functions not struct. This new extension might be only for function forward declartion.

       %10 = OpExtInstWithForwardRefs %void %1 DebugTypeFunction %uint_0 %11
       %12 = OpExtInstWithForwardRefs %void %1 DebugFunction %3 %10 %8 %uint_0 %uint_0 %11 %3 %uint_0 %uint_0

For our case, we may need to use OpTypeForwardPointer that works for struct. I am gonna give a try and see if it works.

jkwak-work commented 1 week ago

Well.. it turned out that OpTypeForwardPointer is for something else. And I also found that OpExtInstWithForwardRefs can be used with DebugTypeComposite from one of test cases.

%33 = OpExtInstWithForwardRefsKHR %void %1 DebugTypeComposite %5 %uint_0 %31 %uint_3 %uint_7 %32 %5 %uint_0 %uint_3 %34
jkwak-work commented 6 days ago

It seems like there was a build break on spirv-header repository and spirv-tools. In order to use OpExtInstWithForwardRefs, we need little more commits than include fixes.

Spirv-header:

commit 2acb319af38d43be3ea76bfabf3998e5281d8d12 Author: Kévin Petit kevin.petit@arm.com Date: Wed Jun 12 16:41:14 2024 +0100 SPV_ARM_cooperative_matrix_layouts (#433)

Spirv-Tools:

commit ce46482db7ab3ea9c52fce832d27ca40b14f8e87 Author: Nathan Gauër brioche@google.com Date: Thu Jun 6 12:17:51 2024 +0200 Add KHR suffix to OpExtInstWithForwardRef opcode. (#5704) The KHR suffix was missing from the published SPIR-V extension. This is now fixed, but requires some patches in SPIRV-Tools.

jkwak-work commented 6 days ago

Currently this task is blocked because I cannot figure out how to build the executables for "aarch64-linux" platform. Waiting for a help from @expipiplus1 for the problem described on the PR comment. https://github.com/shader-slang/slang/pull/4527#issuecomment-2204091088