Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Remove warning about unspecified vkbinding #6085

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

cheneym2
Copy link
Collaborator

It is permitted that users omit explicit [vk::binding] settings on register definitions of HLSL-style, as they are opting into the default implicit mapping, as with DXC.

That does mean register IDs can clash between resources which have different register type but the same register number. For example:

Such a
problem can be resolved either with explicit bindings in source, or using the "-fvk-binding" slangc options which match DXC behavior.

Closes issue #5938

@cheneym2 cheneym2 requested a review from a team as a code owner January 14, 2025 20:24
It is permitted that users omit explicit [vk::binding] settings on
HLSL-style register definitions. They are just opting into the
default implicit mapping, as they would with HLSL->VK in DXC.

That does mean register IDs can clash between resources which have
different register type but the same register number.

Such a problem can be resolved either with explicit bindings
([vk::binding]) in source, or using the "-fvk-binding" slangc options
which match DXC behavior.

Closes issue shader-slang#5938
@cheneym2 cheneym2 force-pushed the acheney/bindingwarning2 branch from 31f8eaa to 90f8d03 Compare January 14, 2025 20:29
@cheneym2
Copy link
Collaborator Author

/format

@cheneym2 cheneym2 added the pr: non-breaking PRs without breaking changes label Jan 14, 2025
@slangbot
Copy link
Contributor

🌈 Formatted, please merge the changes from this PR

{
// If we warn due to invalid bindings and user did not set how to interpret 'hlsl style
// bindings', we should map `register` 1:1 with equivlent vulkan bindings.
// If the user did not set how to interpret 'hlsl style bindings', we should map
Copy link
Collaborator

Choose a reason for hiding this comment

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

Instead of removing the warning completely, let's remove the warning only if hlslToVulkanLayoutOptions is not set or hlslToVulkanLayoutOptions->canInfer(vulkanKind, hlslInfo.space) is false, and amend the warning message to ask the user to either use [vk::binding] or specify -fvk-binding- options.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Will do

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
pr: non-breaking PRs without breaking changes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants