Compile bug: Vulkan fails to compile after f117be1

[MaggotHATE]

Git commit

ece0f5c1771f1835e66900d4168233f0430d819d

Operating systems

Windows

GGML backends

Vulkan

Problem description & steps to reproduce

f117be1 introduced GLSLC check which fails on portable setups, such as w64devkit. Compilation is successful without the check, and inference runs correctly.

First Bad Commit

f117be1

Compile command

vkt-shaders-gen \
        --glslc      glslc \
        --input-dir  base_sampling2/master/ggml/src/ggml-vulkan/vulkan-shaders \
        --output-dir base_sampling2/master/ggml/src/ggml-vulkan \
        --target-hpp base_sampling2/master/ggml/ggml-vulkan-shaders.hpp \
        --target-cpp base_sampling2/master/ggml/ggml-vulkan-shaders.cpp

Relevant log output

Error: glslc not found at glslc

[Acly]

Usually ${Vulkan_GLSLC_EXECUTABLE} is passed as glslc executable, which should be an executable path, not a command. Did you overwrite that manually?

[MaggotHATE]

Usually ${Vulkan_GLSLC_EXECUTABLE} is passed as glslc executable, which should be an executable path, not a command. Did you overwrite that manually?

In a portable, i.e. self-contained setup, it is preferred to keep libraries within internal folders, hence it is passed as a part of that setup, not as a direct executable. In case of w64devkit, glslc is situated in w64devkit\x86_64-w64-mingw32\bin.

[0cc4m]

So if you set --glslc w64devkit\x86_64-w64-mingw32\bin\glslc does it not find it? Is that because of a relative path issue, or because of std::filesystem?

[Acly]

The point is that whereever glslc is located, CMake should find it, and pass its full path to vulkan-shaders-gen. If shader-gen is only used as part of the build, I don't see why that interferes with a portable setup, you don't have to hardcode that path anywhere.

I think letting CMake resolve the executable path rather than doing it inside shaders-gen is generally a good idea for the limited scope where it's used.

[MaggotHATE]

So if you set --glslc w64devkit\x86_64-w64-mingw32\bin\glslc does it not find it? Is that because of a relative path issue, or because of std::filesystem?

I've already tried it - it finds glslc as a file, but compiles with errors, such as

ggml-vulkan-shaders.cpp:10:260: error: 'div_f16_f32_f32_rte_len' was not declared in this scope
   10 | uint64_t div_len[2][2][2][2] = {{{{div_f32_f32_f32_len,div_f32_f32_f32_rte_len,}, {div_f32_f32_f16_len,div_f32_f32_f16_rte_len,}, }, {{div_f32_f16_f32_len,div_f32_f16_f32_rte_len,}, {div_f32_f16_f16_len,div_f32_f16_f16_rte_len,}, }, }, {{{div_f16_f32_f32_len,div_f16_f32_f32_rte_len,}, {div_f16_f32_f16_len,div_f16_f32_f16_rte_len,}, }, {{div_f16_f16_f32_len,div_f16_f16_f32_rte_len,}, {div_f16_f16_f16_len,div_f16_f16_f16_rte_len,}, }, }, };

[MaggotHATE]

I think that Acly has correctly pointed out the issue in their comment: in case of MinGW it should be resolved automatically rather than be treated as an exact executable.

[Acly]

I think that Acly has correctly pointed out the issue in their https://github.com/ggml-org/llama.cpp/pull/17144#issuecomment-3516011408: in case of MinGW it should be resolved automatically rather than be treated as an exact executable.

There is a misunderstanding, I did not mean to imply that. I think it could be a good idea to not resolve the executable in shaders-gen, because usually CMake already does the resolving. This can make shaders-gen less complex and more robust/secure.

I've already tried it - it finds glslc as a file, but compiles with errors, such as

I'm not sure that's related. This file ggml-vulkan-shaders.cpp no longer exists. Maybe leftover from a previous build?

[0cc4m]

I'll look into resolving the path in cmake and removing the shell from vulkan-shaders-gen.

[MaggotHATE]

I think that Acly has correctly pointed out the issue in their #17144 (comment): in case of MinGW it should be resolved automatically rather than be treated as an exact executable.

There is a misunderstanding, I did not mean to imply that. I think it could be a good idea to not resolve the executable in shaders-gen, because usually CMake already does the resolving. This can make shaders-gen less complex and more robust/secure.

I don't use Cmake, it's Makefile. Even if llama.cpp doesn't have Makefile, there's not reason for Cmake to suddenly exclude shell-based resolving.

I've already tried it - it finds glslc as a file, but compiles with errors, such as

I'm not sure that's related. This file ggml-vulkan-shaders.cpp no longer exists. Maybe leftover from a previous build?

It does exist, just in a different form - as separate files per shader. The compilation rules are the same, the code is mostly the same, and I keep my version of vulkan-shaders-gen.cpp up to date with each change on llama.cpp.

[MaggotHATE]

I'll look into resolving the path in cmake and removing the shell from vulkan-shaders-gen.

Can you explain what you mean, please? How should I prepare my setup beforehand?

[0cc4m]

There is a misunderstanding, I did not mean to imply that. I think it could be a good idea to not resolve the executable in shaders-gen, because usually CMake already does the resolving. This can make shaders-gen less complex and more robust/secure.

I don't use Cmake, it's Makefile. Even if llama.cpp doesn't have Makefile, there's not reason for Cmake to suddenly exclude shell-based resolving.

Now I'm getting confused. We do use CMake, which supports multiple build tools. For Linux and for w64devkit it's probably using Make in the background, is that what you mean?

I've already tried it - it finds glslc as a file, but compiles with errors, such as

That would imply an earlier error that explains why the value (in your case div_f16_f32_f32_rte_len) is missing. Can you post that error?

I'm not sure that's related. This file ggml-vulkan-shaders.cpp no longer exists. Maybe leftover from a previous build?

It does exist, just in a different form - as separate files per shader. The compilation rules are the same, the code is mostly the same, and I keep my version of vulkan-shaders-gen.cpp up to date with each change on llama.cpp.

For the purpose of this issue, we do need logs/info from an unmodified build, otherwise it gets really hard to help.

I'll look into resolving the path in cmake and removing the shell from vulkan-shaders-gen.

Can you explain what you mean, please? How should I prepare my setup beforehand?

I don't understand the question. You don't need to prepare anything.

[MaggotHATE]

For the purpose of this issue, we do need logs/info from an unmodified build, otherwise it gets really hard to help.

I've just tested compilation with Cmake through the same w64devkit setup, and it does compile shaders with the new check - however, in CMakeCache.txt I see that it took glslc.exe from Vulkan SDK:

//Path to a program.
Vulkan_GLSLC_EXECUTABLE:FILEPATH=C:/VulkanSDK/1.3.290.0/Bin/glslc.exe

This means that Cmake prioritizes global variable instead of the local setup. As a result, the setup would not be portable.

Now I'm getting confused. We do use CMake, which supports multiple build tools. For Linux and for w64devkit it's probably using Make in the background, is that what you mean?

No, I meant that I use purely Makefile in my custom llama-cli-based application. The commands for shaders-gen application are the same as they were in Makefile from llama.cpp before it was removed.

That would imply an earlier error that explains why the value (in your case div_f16_f32_f32_rte_len) is missing. Can you post that error?

Oh, sorry, I've posted the wrong error. For compilation with --glslc w64devkit\x86_64-w64-mingw32\bin\glslc it was:

ggml_vulkan: Generating and compiling shaders to SPIR-V
Error executing command for matmul_q4_0_f32_aligned_fp32: Error executing command for matmul_f32_f16_aligned_fp32: Failed to create process
Failed to create process
Error executing command for matmul_f32_f16_fp32: Failed to create processError executing command for Error executing command for matmul_bf16_fp32: Failed to create process

and so on for each shader file.

[Acly]

CMake takes glslc from the Vulkan SDK found at VULKAN_SDK environment variable. You can use it to point to a portable Vulkan SDK that you distribute with your build tools.

It's probably less painful than maintaining by hand a custom Makefile.

Oh, sorry, I've posted the wrong error. For compilation with --glslc w64devkit\x86_64-w64-mingw32\bin\glslc it was:

Not quite sure what is causing it. First step to debug would probably be to print the error using Windows API (GetLastError). But if this can't be reproduced with the official build instructions (ie. cmake), it's up to you to investigate that I think.

[MaggotHATE]

It's probably less painful than maintaining by hand a custom Makefile.

Makefile is actually more convenient than Cmake, so that's not the issue here. I can point it to the same variable, probably, but the point is to keep things portable.

Not quite sure what is causing it. First step to debug would probably be to print the error using Windows API (GetLastError). But if this can't be reproduced with the official build instructions (ie. cmake), it's up to you to investigate that I think.

This part is unrelated to the issue and was caused by a proposed ad-hoc solution. The check introduced in https://github.com/ggml-org/llama.cpp/commit/f117be185ef1b76129e51d26676354af253bf664 is causing trouble on an otherwise functional setup, so it's not up to me - the issue is on llama.cpp side. The idea of treating glslc only as a file while still using shell environment is wrong.

[0cc4m]

How about #17219?

[0cc4m]

I guess we can close this @MaggotHATE ?

[MaggotHATE]

Whoops, sorry, forgot about it. Thanks again for the fix!