Skip to content

cmake: fix paths for vulkan shaders compilation on Windows #8573

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

Merged
merged 2 commits into from
Aug 5, 2024

Conversation

stduhpf
Copy link
Contributor

@stduhpf stduhpf commented Jul 18, 2024

Fixes #8562

I hope it's not breaking it for others.

@mofosyne mofosyne added the Review Complexity : Low Trivial changes to code that most beginner devs (or those who want a break) can tackle. e.g. UI fix label Jul 19, 2024
@mstephenson6
Copy link
Contributor

Just got successful Vulkan builds with MSBuild/cl and ninja/clang on Windows 11 x64 with this PR, thank you!

@0cc4m 0cc4m self-assigned this Aug 4, 2024
Copy link
Collaborator

@0cc4m 0cc4m left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me. No issues on Linux. I can't test Windows, but other commenters confirmed that it works there. Thank you.

@0cc4m 0cc4m merged commit e31a4f6 into ggml-org:master Aug 5, 2024
7 checks passed
@MaggotHATE
Copy link
Contributor

Compilation fails on w64Devkit with this update:

error: 'replace' is not a member of 'std'
  487 |             std::replace(path.begin(), path.end(), '/', '\\' );

@Donwulff
Copy link

Donwulff commented Aug 6, 2024

Seconding this, not fixed, from my limited perspective into the code it's bit perplexing because the build seems to be fixed at -std=c++11 while std::replace is c++20 onwards, how is this supposed to work anywhere?

@0cc4m
Copy link
Collaborator

0cc4m commented Aug 6, 2024

Seconding this, not fixed, from my limited perspective into the code it's bit perplexing because the build seems to be fixed at -std=c++11 while std::replace is c++20 onwards, how is this supposed to work anywhere?

I agree the way it is shown by default in the documentation makes it look like std::replace is available since C++17 or C++20, but if you set the C++ revision at the top of the page you can see that std::replace is available since C++98. That's not the issue.

Maybe the problem for w64Devkit is just that #include <algorithm> is missing in the #ifdef _WIN32 include part at the top? Can you try that?

@MaggotHATE
Copy link
Contributor

#include

Yes, it seems to be the case #8880
Although I've missed your comment and didn't put it under #ifdef _WIN32 - is it critical?

@0cc4m
Copy link
Collaborator

0cc4m commented Aug 6, 2024

#include

Yes, it seems to be the case #8880 Although I've missed your comment and didn't put it under #ifdef _WIN32 - is it critical?

Not critical, but the algorithm part is in a Windows-only section.

arthw pushed a commit to arthw/llama.cpp that referenced this pull request Aug 7, 2024
…8573)

* Vulkan-shaders: attempt fix compilation on windows

* fix miss-matched parenthesis
kangalio added a commit to kangalio/llama.cpp that referenced this pull request May 3, 2025
They were shoehorned through a Windows-style string representation, where the entire command line is just a space-separated string of arguments. If any arguments contain spaces, you have to escape them. This was implemented in ggml-org#8573, however, the issue remains on Unix.

Usually, this would NOT be an issue on Unix, because it doesn't have the braindead Windows-style API that only takes a flat string as a command line. In Unix, you can pass the arguments as a legit array. However, Llama.cpp didn't make use of that; instead running `sh -c <the flattened argument string>`. Thereby inflicting the same problems onto itself as Windows has.

Instead of bandaid-fixing this by also slapping quotes around the path arguments (which would also break apart as soon as the path contains quote characters), I decided to fix it properly.

So, the code now doesn't wrap the command line in `sh -c`, but passes the arguments directly, circumventing the need to do any brittle escaping.

This also necessitated replacing the `coopmat ? "" : "-O"` parameter with a proper conditional parameter. Because the empty string parameter (rightfully) confused glslc.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Review Complexity : Low Trivial changes to code that most beginner devs (or those who want a break) can tackle. e.g. UI fix
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Bug: Vulkan build no longer working with MSVC cmake on windows
8 participants