[SM6.10] Update HL LinAlg builtin signatures#8173
Conversation
| void [[min_sm=6.10]] __builtin_LinAlg_MatrixMatrixMultiply(out LinAlgMatrix matrixC, in LinAlgMatrix matrixA, in LinAlgMatrix matrixB); | ||
| void [[min_sm=6.10]] __builtin_LinAlg_MatrixMatrixMultiplyAccumulate(out LinAlgMatrix matrixC, in LinAlgMatrix matrixA, in LinAlgMatrix matrixB); | ||
| void [[min_sm=6.10]] __builtin_LinAlg_MatrixAccumulate(out LinAlgMatrix matrixC, in LinAlgMatrix matrixLHS, in LinAlgMatrix matrixRHS); |
There was a problem hiding this comment.
Is the double Matrix in the name intentional? I think __builtin_LinAlg_MatrixMultiply would be just fine.
And shouldn't these return a Matrix value instead of having an out parameter?
There was a problem hiding this comment.
The double is intentional yes! There is MatrixMatrixMultiply (aka Matrix x Matrix) and MatrixVectorMultiply (aka Matrix x Vector)
Nope, out parameter is necessary for overload resolution! They get translated into the correct return shape in lowering. Since we can overloading on just the return type it needs to be captured in the parameter list
There was a problem hiding this comment.
Do you mean "cannot do overloading on just the return type"?
| void [[min_sm=6.10]] __builtin_LinAlg_MatrixStoreToMemory(in LinAlgMatrix matrix, in int GroupSharedMem, in uint offset, in uint stride, in uint layout); | ||
| uint [[min_sm=6.10]] __builtin_LinAlg_MatrixQueryAccumulatorLayout(); | ||
| void [[min_sm=6.10]] __builtin_LinAlg_MatrixMatrixMultiply(out LinAlgMatrix matrixC, in LinAlgMatrix matrixA, in LinAlgMatrix matrixB); | ||
| void [[min_sm=6.10]] __builtin_LinAlg_MatrixMatrixMultiplyAccumulate(out LinAlgMatrix matrixC, in LinAlgMatrix matrixA, in LinAlgMatrix matrixB); |
There was a problem hiding this comment.
The signature here doesn't seem to indicate that any matrix is "updated" during the accumulation. Should matrixA be an inout, or otherwise?
There was a problem hiding this comment.
So, there is an issue here in that regard but the fix (coming in a later PR)[1] doesn't make this look any clearer. The API is weird that even when something is updated "in place" a new SSA value is produced.
At the DXIL level this function is
declare %dx.types.LinAlgMatrix<mangling> @dx.op.linAlgMatrixMultiplyAccumulate.[MatTyR].[MatTyA].[MatTyB].[MatTyC](
immarg i32, ; opcode
%dx.types.LinAlgMatrix<mangling>, ; matrix A
%dx.types.LinAlgMatrix<mangling> ; matrix B
%dx.types.LinAlgMatrix<mangling> ; matrix C
)so its better to think of the function as R = A * B + C. R is just missing because it was a spec bug that was fixed after this PR was written (and its a lot easier on me to punt the fix instead of doing a bunch of complicated rebases)
1: The final signature of this function will actually be:
void [[min_sm=6.10]] __builtin_LinAlg_MatrixMatrixMultiplyAccumulate(out LinAlgMatrix matrixR, in LinAlgMatrix matrixA, in LinAlgMatrix matrixB, in LinAlgMatrix matrixC);
There was a problem hiding this comment.
Actually the very next PR contains this fix. I suppose I could merge them into one 2-topic PR if folks prefer
There was a problem hiding this comment.
I see, I don't mind doing whatever is easier to you
|
@copilot Can you take a look at the current linalg spec available here:https://github.com/microsoft/hlsl-specs/blob/main/proposals/0035-linalg-matrix.md And then review the changes in this PR. Share your thoughts in a single comment. |
Resolves the `int MatrixRef` TODO left from previous changes, as well as updates most builtins to match their final shape. This should be considered an NFC change since these builtins aren't implemented yet and instead we are just updating the shape of the reversed op slots
Resolves the
int MatrixRefTODO left from previous changes, as well as updates most builtins to match their final shape.This should be considered an NFC change since these builtins aren't implemented yet and instead we are just updating the shape of the reversed op slots