-
Notifications
You must be signed in to change notification settings - Fork 51
[0029][0031]In linalg Mul/MulAdd functions, the memory backing the Matrix and Bias buffers must be read-only #507
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
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't have anything against this apart from the minor quibble about example code comments.
When I took an initial run at this, I had significantly more changes to the API itself as a result of disallowing RWMatrixRefs which results in much better error messsages when RW types are used: pow2clk@ea67c02 Sorry I didn't link this to you sooner.
matrix is stored in memory, while the vector comes from a variable. The Mul | ||
function expects the memory backing the matrix object to be read-only | ||
(for efficient loads and hardware optimizations) and, therefore, the matrix | ||
must be of MatrixRef type (and not RWMatrixRef type). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I expected this to result in changing the interface of these functions so they take MatrixRef and not MatrixRefImpl.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed.
bias vector (loaded from memory) to the result. | ||
bias vector (loaded from memory) to the result. Similar to the the matrix | ||
operand, the memory backing the bias vector must be read-only, and therefore an | ||
object of `VectorRef` type (and not `RWVectorRef` type). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As above except without the ability to specify RWVectorRef here, the type is not needed at all.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1
ByteAddressBuffer inputMatrix0; // note read-only buffer | ||
ByteAddressBuffer inputMatrix1; // note read-only buffer | ||
ByteAddressBuffer biasVector0; // note read-only buffer | ||
ByteAddressBuffer biasVector1; // note read-only buffer |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think one comment at the top of the block pointing out that they are all read-only buffers is sufficient.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Honestly since ByteAddressBuffer
is already a read only buffer I don't think any of these comments are needed. Its belaboring the point
matrix is stored in memory, while the vector comes from a variable. The Mul | ||
function expects the memory backing the matrix object to be read-only | ||
(for efficient loads and hardware optimizations) and, therefore, the matrix | ||
must be of MatrixRef type (and not RWMatrixRef type). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed.
bias vector (loaded from memory) to the result. | ||
bias vector (loaded from memory) to the result. Similar to the the matrix | ||
operand, the memory backing the bias vector must be read-only, and therefore an | ||
object of `VectorRef` type (and not `RWVectorRef` type). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1
Issue: microsoft/DirectXShaderCompiler#7490