Skip to content

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

Closed
cassiebeckley wants to merge 1 commit intomicrosoft:mainfrom
cassiebeckley:spirv-type-builtin
Closed

[SPIR-V] Allow built-in object types as arguments to SpirvType#6951
cassiebeckley wants to merge 1 commit intomicrosoft:mainfrom
cassiebeckley:spirv-type-builtin

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
@cassiebeckley cassiebeckley requested a review from a team as a code owner October 8, 2024 19:00
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.

@github-project-automation github-project-automation bot moved this from New to Done in HLSL Roadmap May 23, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Archived in project

Development

Successfully merging this pull request may close these issues.

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

4 participants