Skip to content

Proposal for mapping resource attributes to DXIL and SPIR-V #76

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

Merged
merged 4 commits into from
Feb 12, 2025

Conversation

bogner
Copy link
Collaborator

@bogner bogner commented Oct 8, 2024

No description provided.

@bogner bogner force-pushed the 2024-10-08-resource-attributes branch from 57e9383 to c412b60 Compare October 8, 2024 18:28
@bogner bogner force-pushed the 2024-10-08-resource-attributes branch from c412b60 to d0b6a34 Compare October 8, 2024 18:54
Comment on lines 252 to 271
| | Dim | Depth | Arrayed | MS | Sampled | Access |
|---------------------------------|--------|-------------|---------|-------|---------|--------|
| Texture1D | 1D | Unknown (2) | false | false | r/o (1) | r/o |
| Texture1DArray | 1D | Unknown (2) | true | false | r/o (1) | r/o |
| Texture2D | 2D | Unknown (2) | false | false | r/o (1) | r/o |
| Texture2DArray | 2D | Unknown (2) | true | false | r/o (1) | r/o |
| Texture2DMS | 2D | Unknown (2) | false | true | r/o (1) | r/o |
| Texture2DMSArray | 2D | Unknown (2) | true | true | r/o (1) | r/o |
| Texture3D | 3D | Unknown (2) | false | false | r/o (1) | r/o |
| TextureCUBE | Cube | Unknown (2) | false | false | r/o (1) | r/o |
| TextureCUBEArray | Cube | Unknown (2) | true | false | r/o (1) | r/o |
| RWTexture1D | 1D | Unknown (2) | false | false | r/w (2) | r/w |
| RWTexture1DArray | 1D | Unknown (2) | true | false | r/w (2) | r/w |
| RWTexture2D | 2D | Unknown (2) | false | false | r/w (2) | r/w |
| RWTexture2DArray | 2D | Unknown (2) | true | false | r/w (2) | r/w |
| RWTexture2DMS | 2D | Unknown (2) | false | true | r/w (2) | r/w |
| RWTexture2DMSArray | 2D | Unknown (2) | true | true | r/w (2) | r/w |
| RWTexture3D | 3D | Unknown (2) | false | false | r/w (2) | r/w |
| Buffer | Buffer | Unknown (2) | false | false | r/o (1) | r/o |
| RWBuffer | Buffer | Unknown (2) | false | false | r/w (2) | r/w |
Copy link
Collaborator

Choose a reason for hiding this comment

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

The Access Qualifier is for Kernel (think OpenCL) only. The value after "Sampled" is the Image Format.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Updated the table to add an Image format column but largely leave it blank. See my replies to your other comments

Comment on lines 273 to 274
DXC lowers [Constant/Texture/Structured/Byte Buffers] to [OpTypeStruct] in
SPIR-V with a uniform storage class and various layout decorations. DXC tracks
Copy link
Collaborator

Choose a reason for hiding this comment

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

The storage class should depend on the version of Vulkan that is targeted. Starting with Vulkan 1.2 the storage class is StorageBuffer.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Updated to be somewhat handwavey about this but mention the version dependency

Copy link
Collaborator

@s-perron s-perron left a comment

Choose a reason for hiding this comment

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

We need to add a section discussing the "Image format" for spir-v. This has been discussed a lot in DXC over the years. The Image format indicates the format of the underlying data. I don't know what the expectations are for DXIL, but many Vulkan users want flexibility. The most flexibility is to use Unknown as much as possible bey default. However for Vulkan we cannot always use Unknown.

There are two feature bits that we care about: shaderStorageImageReadWithoutFormat and shaderStorageImageWriteWithoutFormat. I'm trying to check how available these feature are. However, many devices are still running Vulkan 1.1, and probably do not support it.

DXC guesses at the image format based on the sampled type, and developers can use the vk::image_format attribute to set it to something else. We should document what we will be doing.

Vulkan documentation:

https://docs.vulkan.org/spec/latest/appendices/spirvenv.html#spirvenv-format-type-matching
https://docs.vulkan.org/spec/latest/appendices/spirvenv.html#spirvenv-image-formats

DXC Issues:

microsoft/DirectXShaderCompiler#4941
microsoft/DirectXShaderCompiler#4773
microsoft/DirectXShaderCompiler#2498
microsoft/DirectXShaderCompiler#3395
microsoft/DirectXShaderCompiler#4868

Comment on lines 333 to 348
| Texture1D | SRV | vec4 | - | 1D | - | - | - | - | - |
| Texture1DArray | SRV | vec4 | - | 1D | - | - | yes | - | - |
| Texture2D | SRV | vec4 | - | 2D | - | - | - | - | - |
| Texture2DArray | SRV | vec4 | - | 2D | - | - | yes | - | - |
| Texture2DMS | SRV | vec4 | - | 2D | yes | - | - | - | - |
| Texture2DMSArray | SRV | vec4 | - | 2D | yes | - | yes | - | - |
| Texture3D | SRV | vec4 | - | 3D | - | - | - | - | - |
| TextureCUBE | SRV | vec4 | - | Cube | - | - | - | - | - |
| TextureCUBEArray | SRV | vec4 | - | Cube | - | - | yes | - | - |
| RWTexture1D | UAV | vec4 | - | 1D | - | - | - | - | - |
| RWTexture1DArray | UAV | vec4 | - | 1D | - | - | yes | - | - |
| RWTexture2D | UAV | vec4 | - | 2D | - | - | - | - | - |
| RWTexture2DArray | UAV | vec4 | - | 2D | - | - | yes | - | - |
| RWTexture2DMS | UAV | vec4 | - | 2D | yes | - | - | - | - |
| RWTexture2DMSArray | UAV | vec4 | - | 2D | yes | - | yes | - | - |
| RWTexture3D | UAV | vec4 | - | 3D | - | - | - | - | - |
Copy link
Collaborator

Choose a reason for hiding this comment

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

These look good. The individual attributes map nicely to individual operands to spirv.Image.

Comment on lines 276 to 277
need to keep track of that information in new SPIR-V target extension types in
our implementation.
Copy link
Collaborator

Choose a reason for hiding this comment

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

I agree we will need a new target type.

I've been thinking about this a bit. SPIR-V does not have a "handle" for structured buffers. There is a global variable, which is accessed using the same instructions used to access function scope variables.

In Vulkan SPIR-V, a structured buffer StructuredBuffer<T> is represented as struct { T[] }. We could make the handle a pointer to the struct or a pointer to the array. In any case, you want it to be a pointer type.

One option is to have CreateHandleFromBinding return a pointer. Then use use standard llvm-ir instruction to access it. The problem with that is that we will have to expose the layout of T, and the type information may be hard to recover. That is why I agree we need a target type.

We should be able to define a target type to represent a spir-v pointer.

Comment on lines 359 to 366
| ByteAddressBuffer | SRV | - | - | - | - | - | - | yes | - |
| RWByteAddressBuffer | UAV | - | - | - | - | - | - | yes | - |
| RasterizerOrderedByteAddressBuffer | UAV | - | yes | - | - | - | - | yes | - |
| StructuredBuffer | SRV | struct | - | - | - | - | - | yes | - |
| RWStructuredBuffer | UAV | struct | - | - | - | - | - | yes | - |
| RasterizerOrderedStructuredBuffer | UAV | struct | yes | - | - | - | - | yes | - |
| AppendStructuredBuffer | UAV | struct | - | - | - | - | - | yes | - |
| ConsumeStructuredBuffer | UAV | struct | - | - | - | - | - | yes | - |
Copy link
Collaborator

Choose a reason for hiding this comment

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

To generate or use these types, the SPIR-V backend will have to know the layout of any structs. In DXC, we have a few different layout options. I feel like it should be the FE's job to specify the layout. Let me know what you think.

Comment on lines 359 to 360
| ByteAddressBuffer | SRV | - | - | - | - | - | - | yes | - |
| RWByteAddressBuffer | UAV | - | - | - | - | - | - | yes | - |
Copy link
Collaborator

Choose a reason for hiding this comment

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

ByteAddressBuffers are hard in SPIR-V. Without untyped pointers, we cannot cast from one pointer type to another. This make it very hard to do some thing, and impossible to do others. For, example, it is impossible to have both 32-bit atomic operations and 64-bit atomic operations.

For now, we cannot have a common implementation for all of the raw buffer resources.

Comment on lines 116 to 117
View), CBV (Constant buffer view), or Sampler. The only difference between
SRV and UAV is whether the object is writeable.
Copy link
Collaborator

Choose a reason for hiding this comment

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

The only difference between SRV and UAV is whether the object is writeable.

The resource class also has implications on the register type that the resource can be bound to. I think I'd expect to see some reference to that (maybe just a link to the other documentation we have about it).

I almost wonder if it would be fair to say that the only reason we track this resource class is to know which register types it belongs to. It's notably absent from any discussion about the SPIR-V implementation, where SPIR-V doesn't distinguish between different types of register.

Copy link
Collaborator

Choose a reason for hiding this comment

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

@bogner - thoughts on this?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I reworded this to mention resource class but not really go into detail, WDYT?

- The Resource class is SRV (Shader resource view), UAV (Unordered Access
  View), CBV (Constant buffer view), or Sampler. This is largely only used for
  determining which register class a resource belongs to, and is mostly
  valuable in differentiating between (writeable) UAVs and (read-only) SRVs.

bogner added a commit to bogner/llvm-project that referenced this pull request Dec 12, 2024
Instead of storing an auxilliary structure with the information from the
DXIL resource target extension types duplicated, access the information
that we can via the type itself.

This also means we need to handle some of the target extension types we
haven't fully defined yet, like Texture and CBuffer. For now we make an
educated guess to what those should look like based on llvm/wg-hlsl#76,
and we can update them fairly easily when we've defined them more
thoroughly.

First part of llvm#118400
bogner added a commit to llvm/llvm-project that referenced this pull request Dec 16, 2024
Instead of storing an auxilliary structure with the information from the
DXIL resource target extension types duplicated, access the information
that we can via the type itself.

This also means we need to handle some of the target extension types we
haven't fully defined yet, like Texture and CBuffer. For now we make an
educated guess to what those should look like based on llvm/wg-hlsl#76,
and we can update them fairly easily when we've defined them more
thoroughly.

First part of #118400
bogner added a commit to llvm/llvm-project that referenced this pull request Dec 16, 2024
Instead of storing an auxilliary structure with the information from the
DXIL resource target extension types duplicated, access the information
that we can via the type itself.

This also means we need to handle some of the target extension types we
haven't fully defined yet, like Texture and CBuffer. For now we make an
educated guess to what those should look like based on llvm/wg-hlsl#76,
and we can update them fairly easily when we've defined them more
thoroughly.

First part of #118400
@bogner bogner linked an issue Jan 28, 2025 that may be closed by this pull request
@bogner
Copy link
Collaborator Author

bogner commented Feb 4, 2025

We need to add a section discussing the "Image format" for spir-v. This has been discussed a lot in DXC over the years. The Image format indicates the format of the underlying data. I don't know what the expectations are for DXIL, but many Vulkan users want flexibility. The most flexibility is to use Unknown as much as possible bey default. However for Vulkan we cannot always use Unknown.

I added some of these links and basically portrayed all of this as a work-in-progress. I think we need to have a discussion about where we want the language to go in this regard (it's fairly clear that vk::image_format, while it does the job, is probably not sufficiently forward looking)

@damyanp damyanp self-requested a review February 10, 2025 22:16
@damyanp damyanp assigned damyanp and pow2clk and unassigned tex3d Feb 10, 2025
Copy link
Collaborator

@damyanp damyanp left a comment

Choose a reason for hiding this comment

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

LGTM - don't forget to assign a proper proposal number to this before merging.

@damyanp damyanp removed their assignment Feb 11, 2025
Copy link
Collaborator

@s-perron s-perron left a comment

Choose a reason for hiding this comment

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

LGTM. I'll follow up with more SPIR-V specific details in #98.

@bogner bogner merged commit 06455bf into llvm:main Feb 12, 2025
@bogner bogner deleted the 2024-10-08-resource-attributes branch February 12, 2025 21:56
@damyanp damyanp moved this from Needs Review to Closed in HLSL Support Apr 22, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Closed
Development

Successfully merging this pull request may close these issues.

Document the plan for how resource attributes map to DXIL and SPIR-V
5 participants