-
Notifications
You must be signed in to change notification settings - Fork 13.2k
vulkan : shader development improvements #15993
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
base: master
Are you sure you want to change the base?
Conversation
…h less compilation * instead of embedding SPIRV in the library, read them from the filesystem instead * don't need to recompile any .cpp files when modifying shaders
* allows incremental builds * tracks shader dependencies * requires ninja to be installed
Does this have to be tied to ninja? I'd be happy to have proper incremental builds for the glsl files. Maybe we should have one generated file per glsl source file or something like that. |
Writing a file with a timestamp for each source file would be okay. Adding transitive dependencies from #include directives is some additional work towards reinventing ninja though. I'm actually most worried about all the process and filesystem stuff, it's a quagmire of platform-specific weirdness. The current windows pipe handling is already broken and I'm not looking forward to adding sporadic "Access denied" issues to the list. |
I don't understand why we'd need to track timestamps ourselves rather than use cmake to handle the dependencies. cmake is kind of annoying for generated source files but it's possible to make it work. |
Ah, you only mentioned without ninja, so I assumed keeping compiler invocations inside vulkan-shaders-gen as before. It should be possible to generate a CMake instead of ninja file with custom_command & DEPFILE (last time I did this it was only possible for ninja, but looks like it supports few more generators now). |
8bb1ee4
to
ea2dc96
Compare
Updated to use CMake. It's still behind a cmake option atm. @jeffbolznv @0cc4m let me know if you think it's okay to use cmake as the default way to build shaders, then I can simplify by throwing out the old code. |
I'm okay with always using cmake for this. This is in fact very nice to speed up shader iteration, especially with my 2nd gen EPYC I usually have to wait a long time until I can test a change. Thank you! |
I'll test this out, but it isn't quite what I had in mind. Why can't this all be done as cmake logic in the main makefile - get a list of shader files and generate targets for each of them? This seems like a pretty common thing that shouldn't require generating and running a new cmake file. |
Does not build for me with msvc (with GGML_VULKAN_SHADER_DEV enabled):
|
It can, but since it's not a simple 1-1 relationship between .comp and .spv it requires translating and maintaining all the logic that generates the many variants to CMake.
I use MSVC, but didn't try MSBuild... it should be fixed now. But it will not build shaders in parallel, MSBuild doesn't support that. I'm not sure why you're using it, ninja has been the default in MSVC for years and generally builds much faster. |
I was able to build with the latest commit, but when I run test-backend-ops I got this:
|
Hm, so it didn't build shaders at all? Here is what I'm using to test:
I also tested just doing the first command, opening the .sln in VS IDE to build from there, and running test-backend-ops. Both are extremely slow to build though, so limited practical use... |
Oh, I think I see:
Looks like it embedded an absolute path, but I run the tests on a separate test system with this mapped as the Z drive. |
Right, I went with absolute paths so it can be launched from different places (root dir, build/bin, build/bin/Release), but it means it's not relocatable. Copying to machine with different GPUs sounds like something that might be nice to do though, I can try to add some search path mechanism (try some common relative paths and fall back to absolute if not found) |
I did a proof of concept of what I had in mind. It doesn't require moving the logic to cmake, just invoking the process once per source file and filtering by file name. The incremental build and link times are much better (well, except for mul_mm.comp, but even that one is somewhat improved). Please take a look: master...jeffbolznv:llama.cpp:shader_gen_per_file |
Let's get this merged in some way, it would be quite useful. I don't mind the current version, but what do you think of @jeffbolznv 's proposal @Acly ? |
My initial thought is that I don't like it conceptually, it would be better to have a custom executable that only writes a list of what to build, and let make/ninja build it. Remove the custom process launching code, and let the build-system do what they're good at, so they can see all compilation and distribute them to cores/threads efficiently. I don't like multiple build-system threads launching multiple shader-gen which then launch multiple glslc without coordination. But maybe this is only a theoretical problem. The proposal certainly has some practical advantages, fewer changes and better compatibility with all the CMake generators. I want to test it and see how it compares for full build performance, just didn't have much time to do it lately. I think I'll get around to it in next couple days, so let's keep the PR open for now. |
No worries, take your time. I just didn't want this to disappear in the backlog. |
Created #16341 based on the draft by @jeffbolznv and some of the improvements from this PR. Full builds are slightly slower than this PR, but it's not a big difference and my measurements aren't super precise. I'd close this and go with #16341, mostly because it doesn't need a special CMake option and has the lowest risk of failing on various build setups/platforms. |
Adds a CMake option
GGML_VULKAN_SHADER_DEV
(default: off). If enabled, makes tweaking shaders less painful:So when you make a change to a shader file and build, exactly 1 glslc invocation will run, and no C++ compilation at all. This is faster by many orders of magnitude, and will no doubt enhance vulkan backend developer efficiency by a similar factor.