[vulkan] Fix vector shuffle for Vulkan CodeGen #8621

derek-gerstmann · 2025-04-25T00:14:27Z

CodeGen wasn't handling arbitrary mixing of vectors with different lanes to form a shuffle, causing a cast instruction to be generated with mixed types which generated invalid SPIR-V and caused the driver compiler to fail to compile.

Now, we handle the extraction of each selected lane of each vector argument, and construct a new Vector from the combined composite.

Fixes #8580

Re-enable vector_shuffle correctness test for Vulkan.

mcourteaux

Looks good overall. I have a few points of feedback.
Perhaps, as you took my code from the GPU C codegen for this, I suggest we move this into a helper function:

namespace Internal {
std::vector<std::pair<int, int>> calculate_shuffle_vector_and_lane_indices(const Shuffle *s)
}

And clean up both CodeGen_GPU and Codegen_Vulkan using this helper. All the sanity checks can then also go in there.

src/CodeGen_Vulkan_Dev.cpp

mcourteaux · 2025-04-25T07:55:09Z

src/CodeGen_Vulkan_Dev.cpp

            idx -= vec_lanes;
        }

    } else {


If I'm not overlooking anything, it seems there is no specialized branch for simply joining (i.e., concatenating) vectors. Either put a TODO comment, make an issue, or add that branch. Falling back on the generic vector shuffle, below, seems like a performance waste.

Tracking this here: #8622

mcourteaux · 2025-04-25T08:00:56Z

Also, is this fixable:

Error: Vulkan: Failed to create compute pipeline! vkCreateComputePipelines returned <Unknown Vulkan Result Code>

to actually give a meaningful name to the error code this produced? Khronos spec says:

On failure, this command returns

    VK_ERROR_OUT_OF_HOST_MEMORY
    VK_ERROR_OUT_OF_DEVICE_MEMORY
    VK_ERROR_INVALID_SHADER_NV

I'm guessing I get the last one? Weird that that would be NVIDIA specific.

…dices(). Clean up CodeGen Vulkan Dev comments and asserts.

…ector index mapping now covers this as well.

mcourteaux

Great! Thanks :)

derek-gerstmann · 2025-04-25T21:17:59Z

Also, is this fixable:
Error: Vulkan: Failed to create compute pipeline! vkCreateComputePipelines returned <Unknown Vulkan Result Code>
to actually give a meaningful name to the error code this produced? Khronos spec says:
On failure, this command returns
    VK_ERROR_OUT_OF_HOST_MEMORY
    VK_ERROR_OUT_OF_DEVICE_MEMORY
    VK_ERROR_INVALID_SHADER_NV
I'm guessing I get the last one? Weird that that would be NVIDIA specific.

All of the above are handled in vk_get_error_name(). In this case the return value was -13 (or VK_ERROR_UNKNOWN).

However, if your Vulkan driver supports the debug extension, the runtime registers callbacks to get more information. On my machine I was seeing the following output before this PR:

NVVM compilation failed: 1
Vulkan [WARNING]: (user_context=0x7ffcd1660370, id=2, name:NVIDIA) CreatePipeline: failed to compile internal representation
Vulkan [WARNING]: (user_context=0x7ffcd1660370, id=2, name:NVIDIA) CreatePipeline: unexpected compilation failure
Vulkan [WARNING]: (user_context=0x7ffcd1660370, id=2, name:NVIDIA) CreateComputePipeline: unexpected failure compiling SPIR-V shader: 0x9f5484893ce4c6a
Error: Vulkan: Failed to create compute pipeline! vkCreateComputePipelines returned <Unknown Vulkan Result Code>
Vulkan: Failed to create compute pipeline!
Vulkan: Failed to setup compute pipeline!

mcourteaux · 2025-04-29T07:36:49Z

Ready to land, @derek-gerstmann ?

derek-gerstmann · 2025-04-29T18:37:01Z

Yep all good with me! @abadams any objections to us adding a helper method to the IR Shuffle node?

std::vector<std::pair<int, int>> Shuffle::calculate_vector_and_lane_indices() const;

src/IR.cpp

src/IR.h

src/CodeGen_Vulkan_Dev.cpp

…ethod name. Simplify vector/lane index method to create all indices, and return each pair. Refactor call sites to use `auto vector_and_lane_indices = op->vector_and_lane_indices()`

abadams · 2025-04-30T18:55:20Z

src/CodeGen_Vulkan_Dev.cpp

+
+    // Sanity check that the total lane count matches between the op-type and indices
+    internal_assert(!op->vectors.empty());
+    for (size_t i = 1; i < op->vectors.size(); i++) {


I think shuffle nodes are allowed to combine vectors of different sizes. It looks like the removed code handles this, but this assert looks like it would fail.

Handling that case looks like it was the whole point of the PR, so I must be missing something. Maybe a longer comment here would help.

Yes, good catch! These asserts seem wrong or out of date ... I lifted them from CodeGen_GPU:

https://github.com/halide/Halide/blame/65b08efa78baf964387b9615b4c80e43d4cb967a/src/CodeGen_GPU_Dev.cpp#L149

And the date on those lines is from 3 years ago. So my guess is those asserts were written to match the implementation as it was ... without support to handle vectors of different lanes.

Thanks! I'll remove these lines from both CodeGen_Vulkan and CodeGen_GPU.

We actually aren't triggering those asserts at all. Even the test case seems to produce lowered code that has vectors of the same size for the arguments, but only uses a few lanes from the arguments to produce the shuffle. That case wasn't handled correctly before ... it triggered a type mismatch for the assignments which generated invalid code for mixing vector/scalar types of different sizes.

…rent lanes to be mixed between the arguments and result type.

derek-gerstmann · 2025-05-01T16:56:28Z

Okay to merge? @abadams

derek-gerstmann added 3 commits April 24, 2025 15:08

Fix vector shuffle for CodeGen for Vulkan backend.

2f1bd95

Re-enable vector_shuffle correctness test for Vulkan.

Clang format pass

20a432d

Merge branch 'main' of https://github.com/halide/Halide

0e26d01

derek-gerstmann requested a review from mcourteaux April 25, 2025 00:14

mcourteaux requested changes Apr 25, 2025

View reviewed changes

derek-gerstmann added 5 commits April 25, 2025 12:20

Refactor shuffle index mapping into method on IR node.

7d5e449

Refactor CodeGen GPU Dev to use Shuffle::calculate_vector_and_lane_in…

49f3222

…dices(). Clean up CodeGen Vulkan Dev comments and asserts.

Move some verbose messages to debug(3)

95bb416

Remove special case for handling shuffle to a scalar type since the v…

e8a26aa

…ector index mapping now covers this as well.

Clang format pass

a4d065a

mcourteaux approved these changes Apr 25, 2025

View reviewed changes

Use emplace_back to make clang tidy happy.

c845619

derek-gerstmann requested a review from abadams April 29, 2025 19:32

abadams reviewed Apr 29, 2025

View reviewed changes

src/IR.cpp Outdated Show resolved Hide resolved

abadams reviewed Apr 29, 2025

View reviewed changes

src/IR.h Outdated Show resolved Hide resolved

abadams reviewed Apr 29, 2025

View reviewed changes

src/CodeGen_Vulkan_Dev.cpp Outdated Show resolved Hide resolved

derek-gerstmann added 2 commits April 29, 2025 13:32

Remove calculate_ from Shuffle::calculate_vector_and_lane_indices m…

2f5cf29

…ethod name. Simplify vector/lane index method to create all indices, and return each pair. Refactor call sites to use `auto vector_and_lane_indices = op->vector_and_lane_indices()`

Clang format pass.

3eccf4b

derek-gerstmann requested a review from abadams April 29, 2025 20:39

Trigger Build

a6b083f

abadams reviewed Apr 30, 2025

View reviewed changes

derek-gerstmann added 3 commits April 30, 2025 12:57

Remove stale asserts ... the Shuffle operator allows vectors of diffe…

0eba8a4

…rent lanes to be mixed between the arguments and result type.

Add a more detailed comment on the Shuffle operator and its arguments.

26f3e83

Clang format pass

1003e74

derek-gerstmann requested a review from abadams May 1, 2025 16:56

abadams approved these changes May 1, 2025

View reviewed changes

derek-gerstmann merged commit 82d3aff into main May 1, 2025
19 checks passed

BrewTestBot mentioned this pull request Sep 16, 2025

halide 21.0.0 Homebrew/homebrew-core#244220

Merged

[vulkan] Fix vector shuffle for Vulkan CodeGen #8621

[vulkan] Fix vector shuffle for Vulkan CodeGen #8621

Uh oh!

Conversation

derek-gerstmann commented Apr 25, 2025

Uh oh!

mcourteaux left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

mcourteaux Apr 25, 2025

Choose a reason for hiding this comment

Uh oh!

derek-gerstmann Apr 25, 2025

Choose a reason for hiding this comment

Uh oh!

mcourteaux commented Apr 25, 2025

Uh oh!

mcourteaux left a comment

Choose a reason for hiding this comment

Uh oh!

derek-gerstmann commented Apr 25, 2025

Uh oh!

mcourteaux commented Apr 29, 2025

Uh oh!

derek-gerstmann commented Apr 29, 2025

Uh oh!

Uh oh!

Uh oh!

Uh oh!

abadams Apr 30, 2025

Choose a reason for hiding this comment

Uh oh!

abadams Apr 30, 2025

Choose a reason for hiding this comment

Uh oh!

derek-gerstmann Apr 30, 2025

Choose a reason for hiding this comment

Uh oh!

derek-gerstmann Apr 30, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

derek-gerstmann commented May 1, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

derek-gerstmann Apr 30, 2025 •

edited

Loading