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

[SPIR-V] Allow built-in object types as arguments to SpirvType #6951

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

cassiebeckley
Copy link
Collaborator

There has been a validation error for passing in a built-in type to a template since the first commit to DXC. This check exists to prevent instances like this:

Texture2D<SamplerState> image;

When DXC was first created, users could not create their own templates, and therefore having a generic check for this did not cause any problems. However, now that HLSL 2021 allows users to create their own templates, and inline SPIR-V lets users pass types in to the SpirvType and SpirvOpaqueType templates in order to reference them, this is causing problems.

This PR allows the SpirvType and SpirvOpaqueType templates to take HLSL built-in types as arguments.

Fixes #6498

There has been a validation error for passing in a built-in type to a
template since the first commit to DXC. This check exists to prevent
instances like this:

```
Texture2D<SamplerState> image;
```

When DXC was first created, users could not create their own templates,
and therefore having a generic check for this did not cause any
problems. However, now that HLSL 2021 allows users to create their own
templates, and inline SPIR-V lets users pass types in to the SpirvType
and SpirvOpaqueType templates in order to reference them, this is
causing problems.

This PR allows the SpirvType and SpirvOpaqueType templates to take
HLSL built-in types as arguments.

Fixes microsoft#6498
Copy link
Collaborator

@llvm-beanz llvm-beanz left a comment

Choose a reason for hiding this comment

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

I think we need to have some discussion about this.

Texture types should really only take scalar or vectors. The fact that they ever allowed anything else is a bug.

They do actually work with template objects today (see: https://godbolt.org/z/zPq1T4q3b). The change you're making here allows them to work with HLSL's built-in objects (resources and the like), which absolutely should not be supported.

I'd like to understand better the case where these types should be allowed so that we can better craft language rules around them.

@devshgraphicsprogramming

For me it would just be enough to allow SpirvType to take another SpirvType AND be able to obtain the SpirvType (or some link) to a compound object like groupshared T or RWStructuredBuffer<T>

Stuffing SpirTypes into HLSL templates is probably a road to many bugs, and the most important feature is being able to build SpirvTypes from Native and SpirV Types because that way we can build any type we want/need.

@s-perron
Copy link
Collaborator

Texture types should really only take scalar or vectors. The fact that they ever allowed anything else is a bug.

This should be be allowing more type as templates into the standard Texture types. If it does, that is unintentional.

We want to allow more types as templates to the vk::Spirv*Type. We were thinking we had a use case for allowing textures as a template to be able to define combined texture samplers, but that will not work. We have other issues trying to to implement combined texture samplers as inline spir-v.

However, we should still be able to use a SpirvType as a template to a SpirvType: https://godbolt.org/z/Pqh6nYchf.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: New
Development

Successfully merging this pull request may close these issues.

[SPIR-V] Fix bug using Texture2D with alias template for SpirvType
4 participants