Skip to content

Conversation

@hekota
Copy link
Member

@hekota hekota commented Mar 26, 2025

Test for implicit bindings of resource arrays will need to be added later.

@hekota hekota marked this pull request as ready for review March 26, 2025 23:27
...
#--- end

# UNSUPPORTED: Clang
Copy link
Collaborator

Choose a reason for hiding this comment

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

Vulkan bindings are very different and don't have the same implicit behavior.

Suggested change
# UNSUPPORTED: Clang
# UNSUPPORTED: Clang, Vulkan

Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm going to commit this suggestion to see if it resolves the failing bot, then we can merge this.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Vulkan bindings are very different and don't have the same implicit behavior.

Whiile "It's expected that this test will fail on DXC under Vulkan" is true, I don't think this comment explains why very accurately. There are a couple of things here:

  • DXC doesn't seem to strip unused explicit bindings when targetting Vulcan
  • DXC doesn't fill gaps in bindings compatibly between DX and Vulkan (see also how it handles packoffset)

These are arguably both bugs in DXC, rather than different behaviours of Vulkan. SPIR-V can easily represent the bindings in a matching way to what we emit for DirectX. We can and should make the HLSL behaviour here match between the two targets going forward.

Because of all of this, I think we should probably XFAIL this on DXC-Vulkan rather than call it unsupported, and comment as to why.

Copy link
Collaborator

Choose a reason for hiding this comment

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

My argument against using XFAIL here even though this is pretty clearly a bug is that XFAIL tests always run in case the result changes: we're never going to fix this in DXC, so the result will never change running it just slows down the tests.

I can make it more specific to DXC and add a comment because hopefully we'll do this correctly in Clang.

...
#--- end

# UNSUPPORTED: Clang, Vulkan
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
# UNSUPPORTED: Clang, Vulkan
# UNSUPPORTED: Clang
# This is a bug in DXC's SPIRV that we're almost certainly never going to fix.
# I'm marking this as unsupported rather than xfail because it isn't worth
# running the test just to verify it is still failing.
# UNSUPPORTED: DXC-Vulkan

@hekota hekota merged commit de5d463 into llvm:main Apr 7, 2025
8 of 9 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants