Skip to content
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

Add constant samplers in Vulkan #305

Open
s-perron opened this issue Aug 22, 2024 · 1 comment
Open

Add constant samplers in Vulkan #305

s-perron opened this issue Aug 22, 2024 · 1 comment
Labels
enhancement New feature or request
Milestone

Comments

@s-perron
Copy link
Collaborator

Is your feature request related to a problem? Please describe.

Constant samplers are described in HLSL for DXIL in the root signature. That does not work for Vulkan because constant samplers are treated like constant in the shader using the OpConstantSampler instructions.

We need a way to declare a constnat sampler in Vulkan.

Describe the solution you'd like

Something that would work like https://godbolt.org/z/YKbEa4o43, except the OpConstant instruction would be placed in the correct location in the spir-v.

I'm thinking we could have a builtin function in the vk namespace that would do what that sample ext_instruction does. Then the compiler could put it in the correct place.

Describe alternatives you've considered

See microsoft/DirectXShaderCompiler#4137.

The syntax

[[vk::constant_sampler]]
SamplerState sam
{
	Filter = MIN_MAG_MIP_LINEAR;
	AddressU = Wrap;
	AddressV = Wrap;
};

was suggested. I do not like this because it does not have an obvious mapping to the SPIR-V operands. Also it looks like DXC does not add the information to the AST.

Additional context

Not much to add other than that the original issue seems to have a multiple +1. This could be a useful feature for users.

@crud89
Copy link

crud89 commented Aug 28, 2024

I'd argue that a static initialization syntax for such a domain-specific issue will only fragment the language. Why not have a common attribute that we can attach to SamplerState bindings, that accepts common sampler settings? From what I can tell this could also be implemented without requiring a spec change. Something like this:

[StaticSampler(ADDRESS_REPEAT, FILTER_LINEAR)]
SamplerState sampler : register(s0, space0);

For DXIL, this could then export to reflection, which currently does not support static samplers (and which is a large drawback imho). Of course in D3D static samplers can be set up more granularly, however, if you need such fine control you can still fall back to authoring root signatures. As for SPIR-V, the second parameter of OpConstantSampler dictates if the coordinates are normalized or not. I am not sure if this can be implicitly deduced (I doubt it), but I think a third boolean parameter would do the job that gets ignored when targeting DXIL. Alternatively, I could even see there being multiple overloads of such an attribute with specific settings for D3D static samplers, as well as SPIR-V variants, each with fallbacks to defaults when targeting the other backend (and maybe a warning or two).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
Status: Triaged
Development

No branches or pull requests

2 participants