-
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?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -459,7 +459,10 @@ InterpretedVector<T, N, DT> MakeInterpretedVector(vector<T, N> Vec) { | |
### Function: Mul | ||
|
||
The `dx::linalg::Mul` function performs a matrix-vector multiplication. The | ||
matrix is stored in memory, while the vector comes from a variable. | ||
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 commentThe 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 commentThe reason will be displayed to describe this comment to others. Learn more. Agreed. |
||
|
||
> TODO: add an example for packed types, and make sure they work correctly | ||
|
||
|
@@ -530,7 +533,9 @@ Mul(MatrixRefImpl<MatrixBufferTy, MatrixDT, MatrixM, MatrixK, MatrixLayout, | |
## Function: MulAdd | ||
|
||
The `dx::linalg::MulAdd` function behaves as `dx::linalg::Mul`, but also adds a | ||
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 commentThe 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 commentThe reason will be displayed to describe this comment to others. Learn more. +1 |
||
|
||
Example: | ||
|
||
|
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