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 resource chapter documenting buffers and bindings #344

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

pow2clk
Copy link
Member

@pow2clk pow2clk commented Oct 22, 2024

This adds a resource chapter that documents typed and raw buffers including their methods and compatible operators along with short descriptions of their functionality. It also adds a description of binding annotations for all types. Stub sections are included for constant buffers and samplers, but no information is provided as yet.

Fixes llvm/wg-hlsl#55
Fixes llvm/wg-hlsl#56

This adds a resource chapter that documents typed and raw buffers including their methods and compatible operators along with short descriptions of their functionality. It also adds a description of binding annotations for all types. Stub sections are included for constant buffers and samplers, but no information is provided as yet.

Fixes llvm/wg-hlsl#55
Fixes llvm/wg-hlsl#56
Copy link
Member

@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.

Some initial thoughts.

Comment on lines 7 to 8
Read-only buffers and textures types constitute Shader Resource Views (SRVs) of the external memory.
Writable buffers and textures types constitute Unordered Access Views (UAVs) of the external memory.
Copy link
Member

Choose a reason for hiding this comment

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

I think SRV and UAV are DirectX-isms that we probably want to avoid directly using in the spec language. Maybe just qualifying this as "in DirectX read-only buffers.....SRVs" would help my uneasiness here.

Copy link
Member Author

Choose a reason for hiding this comment

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

Removed them.

All typed buffers can be read through subscript operators or Load methods.
Writable typed buffers can be written through subscript operators.

Typed Buffers are declared with the built-in templates:
Copy link
Member

Choose a reason for hiding this comment

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

Tripped up a little reading this as I expected the templates: to be immediatelly followed by a list of templates.

Dunno if that's me overthinking while reviewing or worth changing though.

Copy link
Member Author

Choose a reason for hiding this comment

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

Should have been removed

void GetDimensions(out uint size);
\end{HLSL}

This returns the full size of the buffer in bytes through the \texttt{width} out parameter.
Copy link
Member

Choose a reason for hiding this comment

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

width

The method definition calls this param size.

Copy link
Member Author

Choose a reason for hiding this comment

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

changed

Comment on lines 97 to 100
The \texttt{status} parameter returns an indication of whether the value retrieved accessed a mapped tile
resource on tile-based platforms. This parameter is interpretted using the \texttt{CheckAccessFullyMapped}
built-in function. If tiled resources are enabled, it returns true if the value was accessed from a mapped
tile resource and false otherwise. If tiled resourced are not enabled, it will return true.
Copy link
Member

Choose a reason for hiding this comment

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

"tile-based platform" is a different thing to tiled resources. Tiled resources aren't really a thing that you enable per se, it's more a property of the underlying resource.

I wonder if we can define this without talking about tiled resources at all?

Maybe just talking about it indicating if the value returned was from mapped memory or not - and as an example referring to D3D12's reserved resources or Vulkan's sparse resources?

Copy link
Member Author

Choose a reason for hiding this comment

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

Left over from when I didn't follow this clearly. removed

RWBuffer<float4> buf;
\end{HLSL}

When defined at global scope, RWBuffers are bound to externally-defined backing stores
Copy link
Member

Choose a reason for hiding this comment

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

Is there some way we can avoid repeating ourselves here? RWBuffer's are exactly like Buffers with some extensions?

Or are there some subtle differences between Load on a Buffer vs RWBuffer? If so then it'd be great if they could more clearly be called out.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think the main difference between RWBuffer and Buffer is that RWBuffer has a non-const operator[] that returns a T&.

That isn't captured here, but we should capture it and probably describe RWBuffer in terms of additions to Buffer. In the Work Graph's spec, I had described the record types with base classes that didn't actually exist as a way of describing the composition of interfaces. That could be a useful tool for resources too.

Copy link
Member Author

Choose a reason for hiding this comment

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

I went back and forth on this. I think we've tied ourselves in knots trying to consolidate common sections of documentation in the past and I noted that the C++ spec repeats a lot of things. Merging information between read-only and writable types might be manageable though. I'll take a pass at it.

Initially, I'm more inclined to merge the sections and just annotated cases where it applies to RW buffers only. Does that sound okay?

Comment on lines 182 to 185
Raw buffers are either ByteAddressBuffers or StructuredBuffers.
ByteAddressBuffers enable per-byte access to the raw data while
StructuredBuffers have an associated structure type that determines how they are
indexed. This type can be a scalar, vector, matrix, or user-defined struct.
Copy link
Member

Choose a reason for hiding this comment

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

I like this summary - I missed having one for typed buffers above.

Copy link
Member Author

Choose a reason for hiding this comment

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

I added something there that I hope is equivalent.

Comment on lines 187 to 190
but partial writes aren't allowed.
Though structured buffers can contain vectors,
assignment to individual vector elements will result in an error.
I HAVE BEEN UNABLE TO VERIFY THIS, THOUGH I WAS SURE IT WAS TRUE!
Copy link
Member

Choose a reason for hiding this comment

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

Could this be something that the frontend makes work for you but this restriction remains at the DXIL level?

This probably needs to be cleaned up before completing the PR.

Copy link
Member Author

Choose a reason for hiding this comment

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

I didn't mean to leave this comment in. Removed.


\begin{HLSL}
ByteAddressBuffer gbuf;
void doit(bool b) {
Copy link
Member

Choose a reason for hiding this comment

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

void doit(bool b)

Wondering why there's the boolean argument in this example?

Copy link
Member Author

Choose a reason for hiding this comment

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

In error from when I had a more involved example. Removed.


\Sub{Constructors}{Resources.rwbabufs.ctrs}

Since their contents are viewed as raw bytes, RWByteAddressBuffers are defined as a simple variable definition.
Copy link
Member

Choose a reason for hiding this comment

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

Wonder if "without any type parameters" might be more specific than "simple variable definition".

Copy link
Member Author

Choose a reason for hiding this comment

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

Replaced

\Sec{Samplers}{Resources.samp}


\Sec{Resource Binding}{Resources.binding}
Copy link
Member

Choose a reason for hiding this comment

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

This needs to have a discussion about bindings on UDT's.

Copy link
Member Author

Choose a reason for hiding this comment

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

Added

Strip mention of UAVs and SRVs

Remove some legacy text mistakenly left in as notes, mischaracterization of tiled memory, leftover sentences from moved or deleted segments, and elements of code examples.

Better summarize typed buffers upfront.

Add description of UDT register annotations.
\texttt{T[]}, or an object of type \texttt{T} where \texttt{T} provides an
the form \texttt{E1[E2]}, \texttt{E1} must either be a variable of array,
vector, matrix, typed buffer (\ref{Resources.tybuf}), or structured buffer
(\ref{Resources.stbufs}), with elements of type \texttt{T[]} or an
Copy link
Collaborator

Choose a reason for hiding this comment

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

Resource types have overloads for operator[] even in DXC. The original language did miss vector and matrix types, but the addition of resources is redundant.

// element access
T operator[](uint pos);
T Load(in int pos);
T Load(in int pos, out uint status);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Some (maybe all) of these methods should probably be const.

\begin{HLSL}
template <typename T = float4>
class Buffer {
Buffer();
Copy link
Collaborator

Choose a reason for hiding this comment

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

We also need a copy constructor.

RWBuffer<float4> buf;
\end{HLSL}

When defined at global scope, RWBuffers are bound to externally-defined backing stores
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think the main difference between RWBuffer and Buffer is that RWBuffer has a non-const operator[] that returns a T&.

That isn't captured here, but we should capture it and probably describe RWBuffer in terms of additions to Buffer. In the Work Graph's spec, I had described the record types with base classes that didn't actually exist as a way of describing the composition of interfaces. That could be a useful tool for resources too.

Copy link
Member Author

@pow2clk pow2clk left a comment

Choose a reason for hiding this comment

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

Responded to Damyan's comments

Comment on lines +85 to +86
T Load(in int pos);
T operator[](uint pos);
Copy link
Member Author

Choose a reason for hiding this comment

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

That's what the documentation says, but it turns out that a -1 makes it through for the subscript in DXC. I don't know how that would be useful though.

RWBuffer<float4> buf;
\end{HLSL}

When defined at global scope, RWBuffers are bound to externally-defined backing stores
Copy link
Member Author

Choose a reason for hiding this comment

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

I went back and forth on this. I think we've tied ourselves in knots trying to consolidate common sections of documentation in the past and I noted that the C++ spec repeats a lot of things. Merging information between read-only and writable types might be manageable though. I'll take a pass at it.

Initially, I'm more inclined to merge the sections and just annotated cases where it applies to RW buffers only. Does that sound okay?


\Sub{Constructors}{Resources.rwbabufs.ctrs}

Since their contents are viewed as raw bytes, RWByteAddressBuffers are defined as a simple variable definition.
Copy link
Member Author

Choose a reason for hiding this comment

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

Replaced

\Sec{Samplers}{Resources.samp}


\Sec{Resource Binding}{Resources.binding}
Copy link
Member Author

Choose a reason for hiding this comment

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

Added

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants