[SM6.10][Exec] Implement Remaining LinAlg Smoke Tests#8366
[SM6.10][Exec] Implement Remaining LinAlg Smoke Tests#8366V-FEXrt wants to merge 20 commits intomicrosoft:mainfrom
Conversation
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
|
Azure Pipelines: Successfully started running 1 pipeline(s). |
|
Azure Pipelines: Successfully started running 1 pipeline(s). |
|
Azure Pipelines: Successfully started running 1 pipeline(s). |
|
Azure Pipelines: Successfully started running 1 pipeline(s). |
|
Azure Pipelines: Successfully started running 1 pipeline(s). |
|
Azure Pipelines: Successfully started running 1 pipeline(s). |
|
Azure Pipelines: Successfully started running 1 pipeline(s). |
|
Azure Pipelines: Successfully started running 1 pipeline(s). |
V-FEXrt
changed the title
|
Azure Pipelines: Successfully started running 1 pipeline(s). |
damyanp
left a comment
damyanp
left a comment
There was a problem hiding this comment.
I haven't carefully reviewed every test, but overall shape etc. LGTM.
|
Azure Pipelines: Successfully started running 1 pipeline(s). |
| addUAVBuffer(Op.get(), "Input", BufferSize, false, "byname"); | ||
| addUAVBuffer(Op.get(), "Output", BufferSize, true); | ||
| addRootUAV(Op.get(), 0, "Input"); | ||
| addRootUAV(Op.get(), 1, "Output"); |
There was a problem hiding this comment.
This says addUAVBuffer and addRootUAV for "Input" when it actually should be an SRV, not a UAV.
Same issue for MatVecMul and MatVecMulAdd
There was a problem hiding this comment.
It looks like the it could be done properly as an SRV by adding another utility function addSRVBuffer which would set the TransitionTo state to D3D12_RESOURCE_STATE_NON_PIXEL_SHADER_RESOURCE | D3D12_RESOURCE_STATE_PIXEL_SHADER_RESOURCE instead of D3D12_RESOURCE_STATE_UNORDERED_ACCESS. (both because an SRV accessible to both can have both flags, according to docs, and we wouldn't want a general addSRVBuffer function to deny PS access).
D3D12_RESOURCE_STATE_NON_PIXEL_SHADER_RESOURCE is used by ShaderOpTest::SetRootValues to determine that this should be an SRV rather than a UAV.
Then you'd probably also want to rename addRootUAV to addRootResourceView or something like that, since it could map CBV, SRV, or UAV, depending on the TransitionTo property in the resource structure.
It might make little difference, but the current code results in a call to SetGraphicsRootUnorderedAccessView instead of SetGraphicsRootShaderResourceView on the command list.
| __builtin_LinAlg_MatrixLoadFromDescriptor( | ||
| Mat, Input, 0, STRIDE, LAYOUT, 128); | ||
|
|
||
| vector<ELEM_TYPE, M_DIM> InVec; |
There was a problem hiding this comment.
Minor: This uses M_DIM for input vector size instead of N_DIM. For this smoke test, M_DIM == N_DIM, so it doesn't really make a difference, but decoupling it might lead to fewer incorrect assumptions based on this example in the future.
Same for MatVecMulAdd.
There was a problem hiding this comment.
I was looking at the spec and I had the impression it would always be M_DIM since its a Column wise multiply?
From context, I'm guessing that changing to ColMajor inverts it to N_DIM?
There was a problem hiding this comment.
It's always an A Matrix, which is MxK. The N_DIM is actually K. For this operation the vector dimension matches the K dim and the output dimension and bias dimension matches M.
4 participants
Implements the remaining smoke tests for the LinAlg DXIL ops
Fixes #8126
Co-authored-by: Copilot 223556219+Copilot@users.noreply.github.com