[SM6.10] Add Matrix class methods and other LinAlg API to linalg.h#8299
[SM6.10] Add Matrix class methods and other LinAlg API to linalg.h#8299hekota merged 10 commits intomicrosoft:mainfrom
Conversation
Implements Matrix class methods and other API function related to linear algebra as specified in LinAlg spec (here)[https://github.com/microsoft/hlsl-specs/blob/main/proposals/0035-linalg-matrix.md#appendix-1-hlsl-header]. The header disables warning for a groupshared argument when the language version is other than 202x. Related to microsoft#7839
af16148 to
c63a4b5
Compare
…less input type interpretation is provided as a separate argument or in InterpretedVector.
…iler into linalg-matrix-header
llvm-beanz
left a comment
There was a problem hiding this comment.
This all looks sane to me. I have a few comments, but nothing that needs to block this PR.
|
|
||
| template <typename T> struct enable_if<true, T> { | ||
| using type = T; | ||
| }; |
There was a problem hiding this comment.
At some point we'll probably want to pull the traits and enable_if out into a separate header so that it can be reused without causing redeclaration issues. For now this is probably fine because I don't think we've really figured out the right way to handle implicit include paths.
tex3d
left a comment
There was a problem hiding this comment.
I think this looks reasonable, though I haven't thoroughly verified that the SFINAE code encodes all the required rules.
It looks like there's a missing use of Transpose in the Cast operation though.
| Load(groupshared T Arr[Size], uint StartIdx, uint Stride, | ||
| MatrixLayoutEnum Layout) { | ||
| Matrix Result; | ||
| __builtin_LinAlg_MatrixLoadFromMemory(Result.__handle, Arr, StartIdx, |
There was a problem hiding this comment.
From the spec:
For the Load operations on groupshared arrays, the Stride argument is the count of elements in the groupshared array.
Should Stride be removed here and replace with Size?
There was a problem hiding this comment.
There might be multiple matrices stored in one array, so Size of the array might not match the Stride. The StartIdx allows loading of different matrices from different places in the array.
There was a problem hiding this comment.
I don’t think it should be replaced with Size, but perhaps it should be clearer what Stride we’re specifying. This isn’t the matrix stride, it’s the row/col stride (whichever is major), right?
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
There was a problem hiding this comment.
I found that inconsistent naming for typename ...Ty vs. ComponentEnum ...Ty made this harder to read and review.
Matrix uses ComponentEnum MatrixDT, which helps, but DT isn't a convention used elsewhere, nor does it match the naming ComponentType which has a long history in HLSL and would better match the ComponentEnum naming.
Could we standardize on naming things with CT when referring to a ComponentType specified by the ComponentEnum and Ty when referring to an actual native type?
To be clear: for unblocking things, we can likely fix the issues I pointed out in a follow-up PR.
| vector<OutputElTy, M> Result; | ||
| __builtin_LinAlg_MatrixVectorMultiply(Result, MatrixA.__handle, Vec, | ||
| __builtin_LinAlg_MatrixVectorMultiply(Result, MatrixA.__handle, | ||
| hlsl::is_signed<OutputElTy>::value, Vec, |
There was a problem hiding this comment.
I believe you need to construct the inputInterp input from the InputElTy , rather than passing in the MatrixDT.
There was a problem hiding this comment.
I think for non-interpreted vectors the assumption is that the interpretation type matches the native scalar type. @llvm-beanz - can you confirm?
| MatrixDT, Bias, MatrixDT); | ||
| __builtin_LinAlg_MatrixVectorMultiplyAdd(Result, MatrixA.__handle, | ||
| hlsl::is_signed<OutputElTy>::value, | ||
| Vec, MatrixDT, Bias, MatrixDT); |
There was a problem hiding this comment.
I believe you need to construct the inputInterp and biasInterp inputs from the InputElTy and BiasElTy, rather than passing in the MatrixDT.
| Result, MatrixA.__handle, InterpVec.Data, InterpVec.Interpretation, Bias, | ||
| MatrixDT); | ||
| Result, MatrixA.__handle, hlsl::is_signed<OutputElTy>::value, | ||
| InterpVec.Data, InterpVec.Interpretation, Bias, MatrixDT); |
There was a problem hiding this comment.
I believe you need to construct the biasInterp input from the BiasElTy, rather than passing in the MatrixDT.
| MatrixDT, BiasVec, BiasElTy); | ||
| __builtin_LinAlg_MatrixVectorMultiplyAdd(Result, MatrixA.__handle, | ||
| hlsl::is_signed<OutputElTy>::value, | ||
| Vec, MatrixDT, BiasVec, BiasElTy); |
There was a problem hiding this comment.
I believe you need to construct the inputInterp input from the InputElTy , rather than passing in the MatrixDT.
I've filed a follow-up issue: #8318 |
Implements
Matrixclass methods and other API functions related to linear algebra as specified in LinAlg spec here.The header disables warning when an argument is marked as
groupsharedwhich is reported when the language version is other than 202x. In order to do that the warning needed to be moved to a separate diagnostic group. Also fixes couple of places in SemaHLSL that did not account for the linalg matrix tyle.Related to #7839