Skip to content

Conversation

ericcurtin
Copy link
Collaborator

@ericcurtin ericcurtin commented Sep 3, 2025

suggest parentheses around ‘&&’ within ‘||’ [-Wparentheses]

@ericcurtin ericcurtin requested a review from 0cc4m as a code owner September 3, 2025 13:32
@ericcurtin ericcurtin requested review from Copilot and removed request for 0cc4m September 3, 2025 13:32
suggest parentheses around ‘&&’ within ‘||’ [-Wparentheses]

Signed-off-by: Eric Curtin <[email protected]>
@ericcurtin ericcurtin force-pushed the vulkan-compiler-warning branch from 6cc3e8b to 7cff721 Compare September 3, 2025 13:33
Copy link

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR fixes a compiler warning about operator precedence in Vulkan code by adding parentheses around the && operation within an || expression to clarify the intended order of operations.

  • Adds parentheses to disambiguate logical operator precedence in a boolean expression

@github-actions github-actions bot added Vulkan Issues specific to the Vulkan backend ggml changes relating to the ggml tensor library for machine learning labels Sep 3, 2025
@ericcurtin
Copy link
Collaborator Author

@0cc4m @taronaeo

(!device->subgroup_size_control && device->subgroup_size >= 16 ||
device->subgroup_size_control && device->subgroup_min_size <= 16 && device->subgroup_max_size >= 16);
((!device->subgroup_size_control && device->subgroup_size >= 16) ||
(device->subgroup_size_control && device->subgroup_min_size <= 16 && device->subgroup_max_size >= 16));
Copy link
Collaborator

Choose a reason for hiding this comment

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

I screwed up the condition again, same thing as with mmid.

It should just be

    const bool use_subgroups16 = use_subgroups && subgroup_min_size_16;

Up to you if you want me to open a PR with that fix and close this PR, or you could update this PR. I haven't tested it yet.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I'll let you do it, you can poke me for a review if you want. I was tempted to look at refactoring:

ggml_vk_load_shaders

function in general, looks like there are some de-duplication opportunities. Or it could at least be split into smaller functions, it's over 1000 lines long.

Copy link
Collaborator Author

@ericcurtin ericcurtin Sep 3, 2025

Choose a reason for hiding this comment

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

Sometimes I don't like lambda expressions because it promotes these long functions. The lambda's could be short static functions with descriptive names (makes for more detailed stack traces too).

@0cc4m 0cc4m closed this Sep 4, 2025
@0cc4m 0cc4m deleted the vulkan-compiler-warning branch September 4, 2025 07:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ggml changes relating to the ggml tensor library for machine learning Vulkan Issues specific to the Vulkan backend

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants