Add resource chapter documenting buffers and bindings #344

pow2clk · 2024-10-22T16:48:31Z

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

damyanp

Some initial thoughts.

specs/language/resources.tex

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.

llvm-beanz · 2024-10-30T15:46:45Z

specs/language/expressions.tex

-\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


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.

llvm-beanz · 2024-10-30T15:48:13Z

specs/language/resources.tex

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


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

llvm-beanz · 2024-10-30T15:49:14Z

specs/language/resources.tex

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


We also need a copy constructor.

Should the copy constructors take references?

Yeah, I was hesitant to suggest references since we don't make them visible to the users, but I suppose that's the way the are in C++ and that's the metaphor we're using here even in cases where it doesn't reflect the actual implementation.

We use references elsewhere and when we do add references to the language I think it'll mean that this documentation just becomes correct.

I think that the copy constructors should take const references though.

specs/language/resources.tex

pow2clk

Responded to Damyan's comments

specs/language/resources.tex

consolidate typed, byte-address, and structured buffers into common sections sharing documentation of their common elements and using inheritance and variant-specific sections to describe differences. Make const all methods that should be Add copy constructors to all buffer types. Revise expressions changes.

pow2clk

Respond to @llvm-beanz comments. The extend of the changes merging many of the buffer descriptions made me unable to respond to some of the comments directly. I used the class inheritance metaphor to represent the relation between some buffer types.

pow2clk · 2024-10-31T22:51:58Z

specs/language/resources.tex

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


pow2clk · 2024-10-31T22:52:39Z

specs/language/resources.tex

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


damyanp

I haven't fully read this yet, but I had a look through the changes that were directly in response to my previous feedback (thanks!)

specs/language/resources.tex

In response to Damyan's comments Corrected some mistaken syntax in buffer declarations. Revise the description of CheckAccessFullyMapped, giving it its own section that other parts refer to. correct sign of parameters to the subscript operator

pow2clk · 2024-11-04T21:12:14Z

specs/language/resources.tex

+   T operator[](int pos) const;
+   T Load(in int pos) const;
+   T Load(in int pos, out uint status) const;


Yup, I was suggesting that we should change the spec here, only call out that this somewhat surprising definition is actually what is intended and isn't a mistake.

Somehow I was unable to respond to this comment from @damyanp about similar lines of text directly. Probably a consequence of the consolidation I did.

I suspect perhaps the intent was to say "I wasn't suggesting that we should change the spec here"? That seems more consistent with the rest of the sentence. I could be persuaded either way, but I've brought all subscript ops and loads into line with a signed integer parameter for now.

Oops, yes, your interpretation is correct.

@bogner - do the parameter types in this spec match what you've/'re implementing?

damyanp · 2024-11-07T00:50:24Z

specs/language/resources.tex

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


Should the copy constructors take references?

damyanp · 2024-11-07T00:51:26Z

specs/language/resources.tex

+   T operator[](int pos) const;
+   T Load(in int pos) const;
+   T Load(in int pos, out uint status) const;


Oops, yes, your interpretation is correct.

@bogner - do the parameter types in this spec match what you've/'re implementing?

specs/language/resources.tex

damyanp · 2024-11-07T00:59:51Z

specs/language/resources.tex

+When defined at local scope, ByteAccessBuffers represent local references
+that can be associated with global ByteAccessBuffers when assigned,
+but must be resolvable to a unique global buffer.


must be resolvable to a unique global buffer

Just checking this is the case. So I couldn't do something like:

ByteAddressBuffer myBuf = globalBuffersArray[threadId];

Resource arrays are different. If you can unambiguously map to the same resource array, that's fine. It's if you cross over into a separate resource entirely that you have problems:

https://godbolt.org/z/9Whzssxr1

ByteAddressBuffer globalBuffersArray[10]; ByteAddressBuffer globalBuffer; RWBuffer<float4> outbuf; [numthreads(8,1,1)] void main(uint threadId: SV_GroupThreadID) { ByteAddressBuffer myBuf; myBuf = globalBuffersArray[threadId]; // This is fine if (threadId > 10) myBuf = globalBuffer; // This is not outbuf[threadId] = myBuf.Load<float4>(0); }

I'll try to come up with some wording that makes this clear and maybe add a note for the specific case

specs/language/resources.tex

damyanp · 2024-11-07T01:55:17Z

specs/language/resources.tex

+  RWBuffer<float> herbuf : register(u0, space1);
+\end{HLSL}
+
+Resource aggregates structs containing multiple resources types or arrays of resource types or structs containing one or more resource types.


Resource aggregates structs containing multiple resources types or arrays of resource types or structs containing one or more resource types.

I might be tired, but I'm having a hard time parsing this one.

I refactored it into a self-referential list of what constitutes an aggregate resource type. I think it's much clearer now.

Corrected a few parameter names and types This led to significant rewording of the atomic operation descriptions. Clarified unique global resource mapping. Refactored description of aggregate resource types for purposes of binding

pow2clk

Thanks Damyan! Responses in relation to the latest commit.

pow2clk · 2024-11-07T19:27:05Z

specs/language/resources.tex

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


Yeah, I was hesitant to suggest references since we don't make them visible to the users, but I suppose that's the way the are in C++ and that's the metaphor we're using here even in cases where it doesn't reflect the actual implementation.

specs/language/resources.tex

pow2clk · 2024-11-07T19:32:16Z

specs/language/resources.tex

+When defined at local scope, ByteAccessBuffers represent local references
+that can be associated with global ByteAccessBuffers when assigned,
+but must be resolvable to a unique global buffer.


Resource arrays are different. If you can unambiguously map to the same resource array, that's fine. It's if you cross over into a separate resource entirely that you have problems:

https://godbolt.org/z/9Whzssxr1

ByteAddressBuffer globalBuffersArray[10]; ByteAddressBuffer globalBuffer; RWBuffer<float4> outbuf; [numthreads(8,1,1)] void main(uint threadId: SV_GroupThreadID) { ByteAddressBuffer myBuf; myBuf = globalBuffersArray[threadId]; // This is fine if (threadId > 10) myBuf = globalBuffer; // This is not outbuf[threadId] = myBuf.Load<float4>(0); }

I'll try to come up with some wording that makes this clear and maybe add a note for the specific case

specs/language/resources.tex

pow2clk · 2024-11-07T19:34:14Z

specs/language/resources.tex

+  RWBuffer<float> herbuf : register(u0, space1);
+\end{HLSL}
+
+Resource aggregates structs containing multiple resources types or arrays of resource types or structs containing one or more resource types.


I refactored it into a self-referential list of what constitutes an aggregate resource type. I think it's much clearer now.

damyanp

LGTM - a few nits in the comments, but I think this is pretty much ready to go in.

damyanp · 2024-11-22T19:31:34Z

specs/language/resources.tex

+  RWBuffer<float> herbuf : register(u0, space1);
+\end{HLSL}
+
+A aggregate resource type can be:


Suggested change

A aggregate resource type can be:

An aggregate resource type can be:

damyanp · 2024-11-22T19:32:52Z

specs/language/resources.tex

+\end{itemize}
+
+When aggregate resource types have register annotations,
+they occupy the first register they specify as well as however many additional sequential registers


Suggested change

they occupy the first register they specify as well as however many additional sequential registers

they occupy the first register they specify, as well as however many additional sequential registers

I don't think a comma is required here. "as well as" serves the same function as "and". In retrospect, I'll change it to "and" for simplicity. I think that makes it clearer that the comma isn't needed since it's just a single conjunction.

damyanp · 2024-11-22T19:34:32Z

specs/language/resources.tex

+
+\begin{note}
+Resource types contained in structs are only allocated registers when they are explicitly used.
+This includes elements of arrays of resources as such array elements must be indexed with literals.


Suggested change

This includes elements of arrays of resources as such array elements must be indexed with literals.

This includes elements of arrays of resources, as such array elements must be indexed with literals.

I also don't think this comma is needed. "As" serves the same purpose as "since", which I will replace it with for similar reasons as above.

This says that if it is a subordinate clause following the main close, no comma is needed. It is needed when it is a subordinate clause that precedes the main clause e.g. "Since such array elements must be indexed with literals, this includes elements of arrays of resources."

damyanp · 2024-11-22T19:37:19Z

specs/language/resources.tex

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


We use references elsewhere and when we do add references to the language I think it'll mean that this documentation just becomes correct.

I think that the copy constructors should take const references though.

pow2clk

I checked with grammarly and copilot to verify my thinking about commas.

pow2clk · 2024-11-25T22:56:19Z

specs/language/resources.tex

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


pow2clk · 2024-11-25T22:56:33Z

specs/language/resources.tex

+\end{itemize}
+
+When aggregate resource types have register annotations,
+they occupy the first register they specify as well as however many additional sequential registers


I don't think a comma is required here. "as well as" serves the same function as "and". In retrospect, I'll change it to "and" for simplicity. I think that makes it clearer that the comma isn't needed since it's just a single conjunction.

pow2clk · 2024-11-25T23:00:14Z

specs/language/resources.tex

+
+\begin{note}
+Resource types contained in structs are only allocated registers when they are explicitly used.
+This includes elements of arrays of resources as such array elements must be indexed with literals.


I also don't think this comma is needed. "As" serves the same purpose as "since", which I will replace it with for similar reasons as above.

This says that if it is a subordinate clause following the main close, no comma is needed. It is needed when it is a subordinate clause that precedes the main clause e.g. "Since such array elements must be indexed with literals, this includes elements of arrays of resources."

pow2clk · 2024-11-25T23:00:26Z

specs/language/resources.tex

+  RWBuffer<float> herbuf : register(u0, space1);
+\end{HLSL}
+
+A aggregate resource type can be:


bharadwajy · 2024-12-02T17:44:58Z

specs/language/resources.tex

+   RWBuffer operator=(RWBuffer buf);
+
+   // element access
+   T &operator[](int n);


Should this be T &operator[](size_t n);?

Along the same lines, should the type of index argument of Buffer::Load(...) methods above be size_t instead of int?

Should this be T &operator[](size_t n);?

Along the same lines, should the type of index argument of Buffer::Load(...) methods above be size_t instead of int?

Never mind. It would contradict the public API

llvm-beanz · 2024-12-06T00:02:48Z

specs/language/resources.tex

+ public:
+   Buffer();
+   Buffer(const Buffer &buf);
+   Buffer operator=(Buffer buf);


Suggested change

Buffer operator=(Buffer buf);

Buffer &operator=(const Buffer &buf);

llvm-beanz · 2024-12-06T00:03:07Z

specs/language/resources.tex

+ public:
+   RWBuffer();
+   RWBuffer(RWBuffer buf);
+   RWBuffer operator=(RWBuffer buf);


Suggested change

RWBuffer operator=(RWBuffer buf);

RWBuffer &operator=(const RWBuffer &buf);

llvm-beanz · 2024-12-06T00:06:05Z

specs/language/resources.tex

+but must be resolvable to a unique global buffer declaration.
+
+\begin{note}
+Local buffers must be resolvable to a unique global declaration, but not necessarily a unique individual buffer.


I find this wording slightly confusing. What is a "unique individual buffer" vs a "unique global declaration"?

I believe this and the next sentence have to do with arrays of resources, but that isn't clear to me from the wording here.

llvm-beanz · 2024-12-06T00:06:55Z

specs/language/resources.tex

+    RWBuffer<int> lrwbuf = grwbuf;
+  }
+\end{HLSL}
+Buffer operands to the assignment operator must have the same element type.


Perhaps more simply:

Buffer types cannot be casted implicitly or explicitly.

The overload resolution of assignment operators isn't impacted.

llvm-beanz · 2024-12-06T00:09:27Z

specs/language/resources.tex

+ public:
+   ByteAddressBuffer();
+   ByteAddressBuffer(const ByteAddressBuffer &buf);
+   ByteAddressBuffer operator=(ByteAddressBuffer buf);


Suggested change

ByteAddressBuffer operator=(ByteAddressBuffer buf);

ByteAddressBuffer &operator=(ByteAddressBuffer &buf);

llvm-beanz · 2024-12-06T00:09:44Z

specs/language/resources.tex

+ public:
+   RWByteAddressBuffer();
+   RWByteAddressBuffer(const RWByteAddressBuffer &buf);
+   RWByteAddressBuffer operator=(RWByteAddressBuffer buf);


Suggested change

RWByteAddressBuffer operator=(RWByteAddressBuffer buf);

RWByteAddressBuffer &operator=(RWByteAddressBuffer &buf);

llvm-beanz · 2024-12-06T00:15:26Z

specs/language/resources.tex

+The alignment requirements of \texttt{offset} for this operation is the size in bytes of the largest
+scalar type contained in the given tyep \texttt{T}.


Suggested change

The alignment requirements of \texttt{offset} for this operation is the size in bytes of the largest

scalar type contained in the given tyep \texttt{T}.

The alignment requirements of \texttt{offset} for this operation is the alignment requirement of an object of type \texttt{T} in the device memory space (\ref{Intro.Memory.Spaces.Device}).

I don't think we should be defining how the alignment requirement of a type is calculated here, instead we should define that elsewhere in the spec. You could add a placeholder reference for something like Intro.Memory.Alignment as well to make sure we have a citation.

llvm-beanz · 2024-12-06T00:25:44Z

specs/language/resources.tex

+The buffer contents is treated as raw data and the casting to the given types
+is done bit for bit.


Take or leave this one, its very nitty.

Suggested change

The buffer contents is treated as raw data and the casting to the given types

is done bit for bit.

The buffer contents is treated as raw data. When accessed the bytes are interpreted as the target type with no conversions or interpolations applied.

llvm-beanz · 2024-12-06T20:04:19Z

specs/language/resources.tex

+   uint DecrementCounter();
+\end{HLSL}
+
+Increment or decrements the hidden counter associated with the RWStructuredBuffer.


What is the return value?

damyanp requested a review from llvm-beanz October 22, 2024 16:56

damyanp reviewed Oct 22, 2024

View reviewed changes

llvm-beanz reviewed Oct 30, 2024

View reviewed changes

pow2clk commented Oct 30, 2024

View reviewed changes

specs/language/resources.tex Outdated Show resolved Hide resolved

specs/language/resources.tex Outdated Show resolved Hide resolved

specs/language/resources.tex Outdated Show resolved Hide resolved

specs/language/resources.tex Show resolved Hide resolved

pow2clk commented Oct 31, 2024

View reviewed changes

damyanp reviewed Nov 1, 2024

View reviewed changes

specs/language/resources.tex Outdated Show resolved Hide resolved

specs/language/resources.tex Outdated Show resolved Hide resolved

specs/language/resources.tex Outdated Show resolved Hide resolved

pow2clk added 2 commits November 1, 2024 16:14

Make headings more consistent, add binding section refs

d5604d8

pow2clk commented Nov 4, 2024

View reviewed changes

damyanp reviewed Nov 7, 2024

View reviewed changes

Respond to Damyan's feedback

93bbd1c

Corrected a few parameter names and types This led to significant rewording of the atomic operation descriptions. Clarified unique global resource mapping. Refactored description of aggregate resource types for purposes of binding

pow2clk commented Nov 7, 2024

View reviewed changes

llvm-beanz added the Design Meeting Agenda item for the design meeting label Nov 12, 2024

damyanp approved these changes Nov 22, 2024

View reviewed changes

constify copy ctrs. Clarify grammar

c5057b9

pow2clk commented Nov 25, 2024

View reviewed changes

damyanp requested a review from llvm-beanz November 25, 2024 23:32

bharadwajy reviewed Dec 2, 2024

View reviewed changes

llvm-beanz reviewed Dec 6, 2024

View reviewed changes

	A aggregate resource type can be:
	An aggregate resource type can be:

	they occupy the first register they specify as well as however many additional sequential registers
	they occupy the first register they specify, as well as however many additional sequential registers

	This includes elements of arrays of resources as such array elements must be indexed with literals.
	This includes elements of arrays of resources, as such array elements must be indexed with literals.

	Buffer operator=(Buffer buf);
	Buffer &operator=(const Buffer &buf);

	RWBuffer operator=(RWBuffer buf);
	RWBuffer &operator=(const RWBuffer &buf);

	ByteAddressBuffer operator=(ByteAddressBuffer buf);
	ByteAddressBuffer &operator=(ByteAddressBuffer &buf);

	RWByteAddressBuffer operator=(RWByteAddressBuffer buf);
	RWByteAddressBuffer &operator=(RWByteAddressBuffer &buf);

		The alignment requirements of \texttt{offset} for this operation is the size in bytes of the largest
		scalar type contained in the given tyep \texttt{T}.

		The buffer contents is treated as raw data and the casting to the given types
		is done bit for bit.

Add resource chapter documenting buffers and bindings #344

Are you sure you want to change the base?

Add resource chapter documenting buffers and bindings #344

Conversation

pow2clk commented Oct 22, 2024

damyanp left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pow2clk left a comment

Choose a reason for hiding this comment

pow2clk left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

damyanp left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pow2clk left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

damyanp left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pow2clk left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment