Skip to content

Vulkan generated targets and shader organization #5356

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

Closed
bandoti opened this issue Feb 6, 2024 · 16 comments
Closed

Vulkan generated targets and shader organization #5356

bandoti opened this issue Feb 6, 2024 · 16 comments
Labels
enhancement New feature or request stale Vulkan Issues specific to the Vulkan backend

Comments

@bandoti
Copy link
Collaborator

bandoti commented Feb 6, 2024

The generated header ggml-vulkan-shaders.hpp is 3 MB of generated binary from the packed Vulkan shaders. These should ideally be generated as a make or CMake target at build time instead of being placed under source control.

In addition, I would like to propose splitting the actual shaders out from ggml_vk_generate_shaders.py. It will likely be easier to reason about the shaders (even though there will be many of them) if they are placed within a separate folder (perhaps $LLAMA_ROOT/vulkan) and then collected/assembled by the python script, instead of having them inline.

That way it will be clearer which part of Vulkan is affected by a given change—and also commit conflicts will be lessened if multiple people are working on separate shaders. In addition, if new shaders need to be added they can simply be dropped in the folder, and the python script may glob them before packing into ggml-vulkan-shaders.hpp.

The hard work to get Vulkan going is greatly appreciated—I look forward to exploring this back-end further. Thank you!

@0cc4m 0cc4m added enhancement New feature or request Vulkan Issues specific to the Vulkan backend and removed bug-unconfirmed labels Feb 6, 2024
@0cc4m
Copy link
Collaborator

0cc4m commented Feb 6, 2024

The idea behind bundling it like this was to allow templating and to keep the number of files in the repo low, which I know @ggerganov prefers. But I see the disadvantages. I'm open to a discussion about what we could replace it with that has as few disadvantages as possible.

There's a few points to consider:

  • GLSL requires a file for each shader
  • GLSL has no direct way for reusing shader code between them (but the Kompute backend has a workaround using the preprocessor)
  • Some custom code is needed to embed the SPIR-V shaders in a header to avoid having to load them on runtime (which Kompute has done in CMake, I think

Also pinging @cebtenzzre who has raised this point before. Let me know what you think.

@ggerganov
Copy link
Member

Moving the shaders to a vulkan-shaders folder similar to kompute-shaders would be fine if that can help to improve the process

@bandoti
Copy link
Collaborator Author

bandoti commented Feb 7, 2024

So here are some basic ideas on this after some consideration.

We could keep ggml-vulkan-shaders.hpp as a facade header which includes one header for each shader. This file itself should be autogenerated and populated with the list of shaders.

In CMake a custom target (using add_custom_command) will be added to the if (LLAMA_VULKAN) ... block which wraps a script invocation—invoking a spirc.py—which does preprocessing and delegates to glslc. Anything with the appropriate shader extensions will subsequently be tracked by this—and will automatically compile these in a *.spirv.hpp file then placed within the build directory, where they can be picked up by ggml-vulkan-shaders.hpp. Similarly, a custom target can be added for ggml-vulkan-shaders.hpp which does something like the following example.

On the make side, logic needs to be added to locate the glslc command. However, the workflow is a bit clearer and essentially something like:

VULKAN_DIR              = ...
VULKAN_COMPILE_CMD      = "$(srcdir)/spirc.py --glslc $(VULKAN_DIR)/glslc"
VULKAN_SHADERS          = $(srcdir)/vulkan-shaders/*.frag ...
VULKAN_SHADERS_COMPILED = $(for s in $(VULKAN_SHADERS); \
        do compiled_name="$(echo "$s" | awk -F/ '{print $NF}' | sed 's/\.frag/\.spirv\.hpp/'))"; \
             echo "$(builddir)/$compiled_name"; done)

$(srcdir)/ggml-vulkan-shaders.hpp: $(VULKAN_SHADERS_COMPILED)
	for c in $(VULKAN_SHADERS_COMPILED); \
		do echo "\#include \"$c\"" >> $@; done

%.spirv.hpp: %.frag
	$(VULKAN_COMPILE_CMD) --input $< --output $@

In this example a separate header extension is used to distinguish these from regular header files which allows the custom rule to be invoked. Either the user is required to specify VULKAN_DIR or some automated mechanism can be added.

I haven't tested any of this code but the flow should at least capture the concept.

@cebtenzzre
Copy link
Collaborator

We could keep ggml-vulkan-shaders.hpp as a facade header which includes one header for each shader. This file itself should be autogenerated and populated with the list of shaders.

For the sake of build times, I would prefer if we built the shaders as a separate translation unit from ggml-vulkan.cpp - at the very least use a precompiled header; at best, have each shader in its own translation unit. The machine I have my AMD GPU in has an aging 16-thread Xeon, and often when developing the Kompute backend I have watched 15 threads sit idle as it recompiles all of the shader headers serially (as part of ggml-kompute.cpp) every time I touch a single line of C++ code.

@pure-water
Copy link

So it appears that I can only have ggm-vulkan-shaders.hpp as an input for now anyway?

@bandoti
Copy link
Collaborator Author

bandoti commented Feb 23, 2024

@0cc4m Let me know if you'd like support on the build scripts. I'm not so much familiar with the Vulkan particulars (e.g. for splitting out the embedded shaders into a compile script) but I'd be happy to help integrate things with Make/CMake. If there's a particular branch let me know and I'll take a look.

@0cc4m
Copy link
Collaborator

0cc4m commented Feb 25, 2024

Eventually, but I'm busy trying to get MoE models to work and I don't really have the capacity to rework the shaders at the same time.

@pure-water
Copy link

is there the stand-alone vulkan shader sub directory now?

@bandoti
Copy link
Collaborator Author

bandoti commented Feb 27, 2024

is there the stand-alone vulkan shader sub directory now?

Not yet. :)

@pure-water
Copy link

Sorry, just made myself right. I would be able to see the shader code as the kcompute-shader one, right?

@bandoti
Copy link
Collaborator Author

bandoti commented Mar 4, 2024

Sorry, just made myself right. I would be able to see the shader code as the kcompute-shader one, right?

That is the goal being discussed here. However, if you are interested in seeing the shaders right now they are packed within ggml_vk_generate_shaders.py. This script is currently used to create the C++ header file ggml-vulkan-shaders.hpp.

@pure-water
Copy link

pure-water commented Mar 12, 2024

OK, thanks. I will update this post after i am done trying to update some shaders to improve some performance on particular network

@github-actions github-actions bot added the stale label Apr 11, 2024
Copy link
Contributor

This issue was closed because it has been inactive for 14 days since being marked as stale.

@0cc4m 0cc4m reopened this Jun 5, 2024
@github-actions github-actions bot removed the stale label Jun 6, 2024
@0cc4m
Copy link
Collaborator

0cc4m commented Jun 13, 2024

@bandoti Sorry this took so long, I'm working on extracting the shader code from the Python file. I'll make a PR that adds those files and adapts the Python file to read them. You could work on replacing the Python file with CMake/Make afterwards.

@bandoti
Copy link
Collaborator Author

bandoti commented Jun 24, 2024

I am working on integrating with the build system now. Will post an update once that's finished.

@0cc4m
Copy link
Collaborator

0cc4m commented Jul 30, 2024

@bandoti I think we can close this issue, right?

@bandoti bandoti closed this as completed Jul 30, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request stale Vulkan Issues specific to the Vulkan backend
Projects
None yet
Development

No branches or pull requests

5 participants