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

[0006] Propose the set of resource attributes that are needed to describe texture resources #42

Closed
wants to merge 5 commits into from

Conversation

bob80905
Copy link
Collaborator

@bob80905 bob80905 commented Aug 19, 2024

This PR proposes a set of attributes that will replace HLSLResourceAttr. This set will be sufficient to distinguish between any texture type, and a table is proposed below that defines which attributes should be present and what values the attributes should have for each texture kind. Certain buffers are also added for extra clarity.

| RasterizerOrderedTexture3D | SRV | yes | 3D | - |
| TextureCUBE | SRV | - | CUBE | - |
| TextureCUBEArray | SRV | - | CUBEArray | - |
| Texture2DMS | SRV | - | 2D | MS |
Copy link
Collaborator

Choose a reason for hiding this comment

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

If we have is_rov, should we have is_multisampled and is_feedback?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

That'll be part of the discussion!

@damyanp
Copy link
Collaborator

damyanp commented Aug 20, 2024

FWIW, I think we've gone too far in the "make the proposal shorter" direction :) It'd be good to get this filled out with more of the proposal stuff. Maybe you were planning this already, it is a draft after all.

A couple of examples of how and where we expect the attributes to appear would be good as well.

| ------------------------------- | -------------------- | ------------ | ----------------------- | ------------------ |
| Texture1D | SRV | - | 1D | - |
| RWTexture1D | UAV | - | 1D | - |
| RasterizerOrderedTexture1D | SRV | yes | 1D | - |
Copy link
Collaborator

Choose a reason for hiding this comment

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

Courtesy of Brian- the ROV resources are UAVs, not SRVs.

@bob80905 bob80905 marked this pull request as ready for review August 20, 2024 21:18
@bob80905 bob80905 changed the title Resource attributes [0006] Propose the set of resource attributes that are needed to describe texture resources Aug 20, 2024
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.

Before we can accept this I would like to see some indication of how these attributes will be used.

Take, for example, hlsl::texture_dimension - what will this be used for?

If one of the uses of it is to determine things about the operator[]'s that are available, how will this work for buffers? Are they a special case?

A bit of me suspects that it might make sense to make the texture_dimension thing be more of a resource_dimension - and so buffers will be 1D resources, like 1D textures are. 

Note though: the change I'm requesting here is to fix the entry for the buffers in the table. I think fixing that would be enough to merge this PR, but we'll need some follow up work in order to get the proposal ready to be accepted.

Comment on lines 73 to 75
| TypedBuffer | SRV | - | - | - | - | yes | yes |
| RawBuffer | SRV | - | - | - | - | yes | yes |
| StructuredBuffer | SRV | - | - | - | - | yes | yes |
Copy link
Collaborator

Choose a reason for hiding this comment

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

These buffer types all have is_feedback and is_array listed on them. Is that intended? (I don't think it should be on them)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Ok, I've adjusted the buffer entries.

@bob80905
Copy link
Collaborator Author

bob80905 commented Aug 23, 2024

Before we can accept this I would like to see some indication of how these attributes will be used.

Take, for example, hlsl::texture_dimension - what will this be used for?

If one of the uses of it is to determine things about the operator[]'s that are available, how will this work for buffers? Are they a special case?

A bit of me suspects that it might make sense to make the texture_dimension thing be more of a resource_dimension - and so buffers will be 1D resources, like 1D textures are. 

Note though: the change I'm requesting here is to fix the entry for the buffers in the table. I think fixing that would be enough to merge this PR, but we'll need some follow up work in order to get the proposal ready to be accepted.

The texture dimension preserves the dimension, and may be used in the future to distinguish between the right load/store overloads to use for the handle.
Though this use isn't currently set in stone, it was briefly proposed as an idea here:
llvm/llvm-project@4be4392#diff-5d235a31f5df8cd876e698f807bb6042678a30a6e1ea6f5649e879a8914504edR167

@bogner will solidify the handle creation design, but has stated before that it's important to capture all information that could be useful in constructing the handle, and dimension seems to be useful.

One advantage of keeping this attribute around, however, is to use it to distinguish between textures and buffers. If we assume this dimension attribute won't exist on buffers, we can use that information to infer the resource kind.

@damyanp
Copy link
Collaborator

damyanp commented Aug 27, 2024

I'm not sure that creating a bunch of attributes to store things that we think might be useful really counts as designing a feature. How can we evaluate the quality of the design of the attribute if we don't know what problem it is solving?

For example, how will the implementation of GetDimensions() make use of these attributes? How about the various Atomic* operations?

Will these need to have to special case something for buffers, when it could be something that falls out naturally from the definition of them? If they are special cases, what attribute does it look at to figure this out?

Maybe it won't, but at the moment this proposal tells us nothing about that.

Comment on lines +73 to +75
| TypedBuffer | SRV | - | - | - | - | - | - |
| RawBuffer | SRV | - | - | - | - | - | - |
| StructuredBuffer | SRV | - | - | - | - | - | - |
Copy link
Collaborator

Choose a reason for hiding this comment

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

The three buffer types are indistinguishable according to these attributes, and I expect we want to cover the missing ConstantBuffer and TextureBuffer types here as well.

Here are some ideas for extending the attributes to distinguish buffer types and incorporate ConstantBuffer/TextureBuffer:

  • Add [[hlsl::row_layout]] for ConstantBuffer and TextureBuffer to identify where special cbuffer row layout rules are used with untranslated components from the constant buffer or typed buffer.
  • [[hlsl::row_layout]] would be combined with [[hlsl::resource_class(CBV)]] for ConstantBuffer or [[hlsl::resource_class(SRV)]] for TextureBuffer.
  • Add [[hlsl::raw_buffer]] for RawBuffer and StructuredBuffer.
  • Add [[hlsl::struct_stride(n)]] for StructuredBuffer, where n is the stride for the structured buffer in bytes.

@llvm-beanz
Copy link
Collaborator

Closing as subsumed by #68

@llvm-beanz llvm-beanz closed this Nov 7, 2024
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.

4 participants