WIP: Offline Shaders (Without Variants) #950

tomadamatkinson · 2024-02-27T01:35:26Z

Description

As discussed on the 27/02/2024 working group call. The current offline shader approaches are overly complex due to the need to support for variants. Removing variants and simplifying the performance samples use of shaders may reduce the complexity of our shader compilation system.

This PR achieves offline compilation through a CMake helper. It also runs spirv-val to validate the spirv after it is created. For now the .spv files are not added to the repository, this may change if we agree on where they should be stored (build dir or next to source).

The PR enables offline shaders for existing ApiVulkanSamples by altering load_shader

After a configure and build the following commands have been tested on desktop

<path/to>/vulkan_samples batch -C api
<path/to>/vulkan_samples batch -C extensions
<path/to>/vulkan_samples batch -C general
<path/to>/vulkan_samples batch -C tooling

Performance samples work by attempting to find a precompiled shader before loading the source in ShaderSource. Variants have been removed by reworking shaders in samples

<path/to>/vulkan_samples batch -C performance

Todo:

constant_data
msaa

Fixes #

General Checklist:

Please ensure the following points are checked:

Note: The Samples CI runs a number of checks including:

I have updated the header Copyright to reflect the current year (CI build will fail if Copyright is out of date)
My changes build on Windows, Linux, macOS and Android. Otherwise I have documented any exceptions

Sample Checklist

If your PR contains a new or modified sample, these further checks must be carried out in addition to the General Checklist:

I have tested the sample on at least one compliant Vulkan implementation
If the sample is vendor-specific, I have tagged it appropriately
I have stated on what implementation the sample has been tested so that others can test on different implementations and platforms
Any dependent assets have been merged and published in downstream modules
For new samples, I have added a paragraph with a summary to the appropriate chapter in the readme of the folder that the sample belongs to e.g. api samples readme
For new samples, I have added a tutorial README.md file to guide users through what they need to know to implement code using this feature. For example, see conditional_rendering
For new samples, I have added a link to the Antora navigation so that the sample will be listed at the Vulkan documentation site

gary-sweet · 2024-02-27T09:12:52Z

bldsys/cmake/shader_helper.cmake

+    find_program(SPV_VALIDATOR_EXECUTABLE spirv-val HINTS $ENV{VULKAN_SDK}/bin)
+    if(NOT SPV_VALIDATOR_EXECUTABLE)
+        message(WARNING "spirv-val not found! SPIR-V validation will not be performed. Please set VULKAN_SDK to the Vulkan SDK path.")
+    endif()
+
+    find_program(GLSLC_EXECUTABLE glslc HINTS $ENV{VULKAN_SDK}/bin)
+    if(NOT GLSLC_EXECUTABLE)
+        message(FATAL_ERROR "glslc not found! Shaders with .glsl extension will not be compiled to SPIR-V. Please set VULKAN_SDK to the Vulkan SDK path.")
+    endif()


So, this won't work that well for us, we don't generally have the Vulkan-SDK on our build servers.
Could we not just add glslangValidator and spirv-tools as submodules and build these as required?

If we bundle the SPIRV with the repository instead of ignoring it then there wouldn't be a need to install any tools out of the box? Tools would only be needed if you change the shaders

Is there any capability for you to install tools on your build servers before running a build task? Possibly with a script?

If we bundle the SPIRV with the repository instead of ignoring it then there wouldn't be a need to install any tools out of the box?

If they're bundled that's fine.

Is there any capability for you to install tools on your build servers before running a build task?

Anything is possible, I'm just looking for the path of least resistance.

Bundling is likely best here. Only maintainers of the shaders would require the tools needed to compile the shaders

There are only a few shaders which have variants (<5) I'm hoping to be able to simplify them further to remove the need entirely. That would mean that we would only have a single SPIRV per shader

Wait, does that imply that building SPIR-V at build time when Sanders change locally is no longer a feature of this design? Right now if someone changes a shader and reruns an app they see the results of the change. Losing that seems like it would be a big deal.

Or is the idea that the bundled code only gets used as a fallback?

It just means that if you do want to alter the shaders you should at least have the tools installed to offline compile shaders. Most users should have this already with the Vulkan SDK

From the discussions in the working group the rough requirements are:

Include HLSL alternatives for all shaders in the long run

Do not include DXC or shader compilers within the project as these are assumed to be installed locally

Prefer offline compilation over online compilation due to the point above

Keep things simple with a single compile pass and document this for users

Previous attempts at solving this have been deemed too complex, hence this approach is all about being simple and removing complexity / unneeded shader usage.

Just updated the todo, I have a few samples left to simplify. Most samples work out of the box with this simpler model

Current bench mark for recompiling all shaders

cmake --build build --target vkb__shaders --parallel 0.98s user 0.91s system 456% cpu 0.416 total

jherico · 2024-02-28T00:40:34Z

framework/common/logging.h

@@ -31,6 +31,8 @@
 #define __FILENAME__ (static_cast<const char *>(__FILE__) + ROOT_PATH_SIZE)

 #define LOGI(...) spdlog::info(__VA_ARGS__);
-#define LOGW(...) spdlog::warn(__VA_ARGS__);
+#define LOGW(...) void(0);


did you mean to leave this in here?

For now yes, helps clean the logs up whilst testing shader changes. This will be reverted when the PR is out of WIP

Will leave this comment unresolved as a reminder

jherico · 2024-02-28T05:55:17Z

I put all my suggestions into tomadamatkinson#3

tomadamatkinson · 2024-02-28T08:19:37Z

Thanks @jherico, if you are open to co-authoring I can create a list of todos and we can split up the remaining work on this PR.

As stated on your created PR I'm happy to merge this after I've given it a spin, it looks to have cleaned up some of the more messy cmake!

jherico · 2024-03-01T17:20:27Z

@tomadamatkinson Sure. Let me know anything you need.

SaschaWillems · 2024-03-01T17:25:06Z

A quick note that I started adding HLSL shaders for some of the samples. We decided to do that first before adding a way to compile those in an automated way. Just wanted to let you know and it would be great if we could defer this until HLSL shaders are in.

jherico · 2024-03-01T17:30:57Z

@SaschaWillems Does that imply are you adding the SHADER_FILES_HLSL parameter to the CMake macros? Even if they don't get precompiled in your branch right now, having the CMake code be able to differentiate between HLSL and GLSL will save the work of a lot of tiny changes in this eventual PR.

SaschaWillems · 2024-03-01T17:37:15Z

Right now it mostly means I have started evaluating how to get them in. I just started on that branch like a few minutes ago, so anything in it is very much non-final. But we decided to do it similar to how HLSL support was added to my samples.

There will also be a way to compile the shaders in that branch.

But it's very very early...

tomadamatkinson · 2024-03-01T18:03:05Z

I'm confused. This branch already supports HLSL shaders. The difficulty with previous approaches was supporting variants. Which we decided not to go forward with. This branch also removes variants from shaders with only a handful of samples left to patch.

Are you stating that you are also not happy with the approach of this revised branch which follows the guidance given in the working group? If so which parts of this branch are not solved how you imagined they would be solved?

SaschaWillems · 2024-03-01T18:05:53Z

I started adding actual HLSL shaders the way we decided during our calls.

I'm NOT going to add offline shader compilation. So imo your PR is still valid and what I'm doing now won't have much effect on your PR aside from maybe some minor things. Your PR is in mind the second step after we added actual shaders and need an easy way to compile them for other maintainers.

Afair we wanted to first add in HLSL shaders and a way of loading SPIR-V generated from them (which is now possible thanks to Mobica's PR) and then add offline compilation for GLSL and HLSL at a later point.

That also matches the proprosal for getting HLSL support into our samples I did some time ago.

But a general plea: Can we lower the pace a bit. Right now things are moving so fast, and even just starting to make some code public starts of discussions. I'm doing all of this in my spare time after a hard work week and tbh. it really stresses me out not being able to work on anything without having to discuss it or trying to clear up confusion :(

… on demand

…rebuild on demand

gary-sweet reviewed Feb 27, 2024

View reviewed changes

tomadamatkinson changed the title ~~WIP: Offline Shaders (Without Variants~~ WIP: Offline Shaders (Without Variants) Feb 27, 2024

jherico reviewed Feb 28, 2024

View reviewed changes

SaschaWillems mentioned this pull request Mar 2, 2024

Add HLSL shaders and improve tooling #961

Open

12 tasks

tomadamatkinson and others added 14 commits March 4, 2024 21:42

feat: Compile shaders offline

0ddeb7c

Only compile shaders if dxc or glslc are available

7523d3a

simplify script

4175ca5

remove shaders

41c2a7d

ignore spv

99af35e

spirv validation

d6d0296

remove redundant sample definition function

df5ee44

correct spirv versions

489620a

fix extensions

86132b0

add spirv support to ShaderSource

90b34f6

support performance samples

b92b78a

Make shader files part of their corresponding build projects, rebuild…

c2d1927

… on demand

fixup! Make shader files part of their corresponding build projects, …

63eab0f

…rebuild on demand

add missing shaders

b03c184

tomadamatkinson force-pushed the offline-shaders branch from 21fda7d to b03c184 Compare March 4, 2024 22:10

jherico mentioned this pull request Mar 18, 2024

Add offline compilation for HLSL shaders with usage of DXC #990

Merged

19 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

WIP: Offline Shaders (Without Variants) #950

WIP: Offline Shaders (Without Variants) #950

tomadamatkinson commented Feb 27, 2024 •

edited

gary-sweet Feb 27, 2024

tomadamatkinson Feb 27, 2024

gary-sweet Feb 27, 2024

tomadamatkinson Feb 27, 2024

jherico Feb 27, 2024

tomadamatkinson Feb 28, 2024

jherico Feb 28, 2024

tomadamatkinson Feb 28, 2024

jherico commented Feb 28, 2024

tomadamatkinson commented Feb 28, 2024

jherico commented Mar 1, 2024

SaschaWillems commented Mar 1, 2024

jherico commented Mar 1, 2024

SaschaWillems commented Mar 1, 2024 •

edited

tomadamatkinson commented Mar 1, 2024

SaschaWillems commented Mar 1, 2024 •

edited

WIP: Offline Shaders (Without Variants) #950

Are you sure you want to change the base?

WIP: Offline Shaders (Without Variants) #950

Conversation

tomadamatkinson commented Feb 27, 2024 • edited

Description

General Checklist:

Sample Checklist

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jherico commented Feb 28, 2024

tomadamatkinson commented Feb 28, 2024

jherico commented Mar 1, 2024

SaschaWillems commented Mar 1, 2024

jherico commented Mar 1, 2024

SaschaWillems commented Mar 1, 2024 • edited

tomadamatkinson commented Mar 1, 2024

SaschaWillems commented Mar 1, 2024 • edited

tomadamatkinson commented Feb 27, 2024 •

edited

SaschaWillems commented Mar 1, 2024 •

edited

SaschaWillems commented Mar 1, 2024 •

edited