slang icon indicating copy to clipboard operation
slang copied to clipboard

Support OpDebugTypePointer for struct member pointer

Open jkwak-work opened this issue 1 year ago • 4 comments

This change supports OpDebugTypePointer for a member variable whose type is a pointer type for the outer struct that hasn't been declared yet. It is done with new extension, "SPV_KHR_relaxed_extended_instruction", that comes with a new instruction, "OpExtInstWithForwardRefs".

Closes #4304

Following external submodules had to be also modified.

external/spirv-header:

commit 2acb319af38d43be3ea76bfabf3998e5281d8d12 Author: Kévin Petit [email protected] Date: Wed Jun 12 16:41:14 2024 +0100 SPV_ARM_cooperative_matrix_layouts (https://github.com/shader-slang/slang/pull/433)

external/spirv-tools:

commit ce46482db7ab3ea9c52fce832d27ca40b14f8e87 Author: Nathan Gauër [email protected] 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.

external/spirv-tools-generated: This is generated from spirv-tools

jkwak-work avatar Jul 02 '24 03:07 jkwak-work

Test is still failing. I think I need to submit a binary of glslang.dll to fix it

jkwak-work avatar Jul 02 '24 04:07 jkwak-work

slang-glslang is up-to-date but two tests are still failing. It looks like I need to update external/slang-binary for spirv-dis.exe and spirv-val.exe I am using a Windows machine and I am not sure how to compile them for linux and aarch64

jkwak-work avatar Jul 02 '24 07:07 jkwak-work

I figured out how to build the executables in external/slang-binaries/spirv-tools with flake.nix. I installed Nix on my WSL with the following command,

curl --proto '=https' --tlsv1.2 -sSf -L https://install.determinate.systems/nix | sh -s -- install

And I was able to build each executables with the following commands.

windows-x64: nix build .#packages.x86_64-linux.cross.mingwW64
windows-x86: nix build .#packages.x86_64-linux.cross.mingw32
x86_64-linux: nix build .#packages.x86_64-linux.static
i686-linux: nix build .#packages.i686-linux.static

But I cannot figure out how to build for "aarch64-linux" I expect that the following command would work, but it doesn't on my WSL,

$ nix build .#packages.aarch64-linux.static
error: a 'aarch64-linux' with features {} is required to build '/nix/store/yqxn0i0sw6gmzxvb8zhfjxdp2cxgx2vi-spirv-tools-static-aarch64-unknown-linux-musl-2023.3.rc1.drv', but I am a 'x86_64-linux' with features {benchmark, big-parallel, kvm, nixos-test, uid-range}

jkwak-work avatar Jul 02 '24 19:07 jkwak-work

Adding boot.binfmt.emulatedSystems = [ "aarch64-linux" ]; to the nixos system config will allow you to run aarch64 binaries and perform a "native" build. nix build .#static --system aarch64-linux

expipiplus1 avatar Jul 02 '24 23:07 expipiplus1

We need to update Vulkan sdk on our testing machines so they don't fail the new test.

csyonghe avatar Jul 04 '24 08:07 csyonghe

We need to update Vulkan sdk on our testing machines so they don't fail the new test.

Yes, I am planning to do that on Monday.

jkwak-work avatar Jul 05 '24 07:07 jkwak-work

Thanks to @expipiplus1 , I was able to figure out how to build and update slang-binaries for spirv-tools. However, the "nix" method we use for building spirv-tools doesn't allow me to use local source files of spirv-tools. I need to use a specific commit of spirv-tool to make this PR working and I am gonna have to find another method to build them.

jkwak-work avatar Jul 09 '24 18:07 jkwak-work

@jkwak-work it is fine to just build spirv-tools locally and manually distribute it to our test servers to allow the test to pass.

csyonghe avatar Jul 09 '24 22:07 csyonghe

@jkwak-work it is fine to just build spirv-tools locally and manually distribute it to our test servers to allow the test to pass.

That may work for CI, but the new test will fail on the local machine if any of us run it locally.

jkwak-work avatar Jul 10 '24 03:07 jkwak-work

I was trying to figure out how spirv-val.exe is used. It appears that external/slang-binaries/spirv-tools/windows-x64/bin/spirv-val.exe is currently not in use. In fact, none of spirv-XXX executables under external/slang-binaries/spirv-tools is used anywhere.

The logic on Slang.dll side is pretty simple and it can be found from here

  1. check if an environment variable, "LANG_RUN_SPIRV_VALIDATION", is set or not.
  2. check if "--skip-spirv-validation" is set.
  3. when 1 is true and 2 is false, it runs "spirv-val XXX" command, and it completely relies on PATH setting to find where "spirv-val" is.
  4. if the validation fails, prints an error message, "internal error 99999: Validation of generated SPIR-V failed."

I think this is not an expected behavior and it is probably a bug; I will make a new issue for this. We should use spirv-val and spirv-dis executables from external/slang-binaries. It can be fixed by one of three ways,

  1. we can update the document and require the users to set Path to include external/slang-binaries/....
  2. or, cmake can copy them from external/slang-binaries to build/XXX/bin directories.
  3. or, we can copy them to a new directory build/spirv-tools, and let slang search for them in a certain order like ".", "..", and Path.

jkwak-work avatar Jul 11 '24 05:07 jkwak-work

I created a new issue for the problem I described on my previous comment. https://github.com/shader-slang/slang/issues/4610

jkwak-work avatar Jul 11 '24 05:07 jkwak-work

We may already have spirv validation linked into slang-glslang.dll.

An alternative is just to find and load this dll, find the spirv validation function exported in the dll(if not exported we can make it so), then invoke that function.

csyonghe avatar Jul 11 '24 06:07 csyonghe

We may already have spirv validation linked into slang-glslang.dll.

An alternative is just to find and load this dll, find the spirv validation function exported in the dll(if not exported we can make it so), then invoke that function.

That is a good idea and it will be more ideal if the functionality can be directly called from another DLL rather than asking the OS to spawn a new process. I will see if slang-glslang.dll can be compiled with the ToT of spirv-tool source code.

jkwak-work avatar Jul 11 '24 18:07 jkwak-work

Changing the status to a draft, because I need to do some rework based on the recent restructuring.

jkwak-work avatar Jul 18 '24 02:07 jkwak-work

CI tests are failing for release builds on all platforms while the debug builds are passing. I may need to break the PR into two PRs. One for the updating spirv-headers/tools and another for the slang side changes.

jkwak-work avatar Jul 18 '24 14:07 jkwak-work

The reason of CI failures is that some of ray-tracing results are different now with the new spirv-header/tools. As an example, tests/vkray/intersection.slang emits the following code,

OpEntryPoint IntersectionKHR %main "main" %a %34 %U

And we expect it to be the following,

// CHECK: OpEntryPoint IntersectionNV %main "main"

I guess this is expected and I should change the tests.

jkwak-work avatar Jul 18 '24 15:07 jkwak-work

Finally all issues are resolved.

The related SPIRV-header/tools are submitted as a separate PR. This leaves this PR to the minimal changes for the issue itself.

Since I got an approval previously and the code hasn't been changed at all, I am going to submit the change soon

jkwak-work avatar Jul 18 '24 21:07 jkwak-work