Skip to content

[SM6.10][Bugfix][Exec] Final test tweaks for preview#8393

Merged
V-FEXrt merged 4 commits intomicrosoft:mainfrom
V-FEXrt:linalg-smoke-preview
Apr 21, 2026
Merged

[SM6.10][Bugfix][Exec] Final test tweaks for preview#8393
V-FEXrt merged 4 commits intomicrosoft:mainfrom
V-FEXrt:linalg-smoke-preview

Conversation

@V-FEXrt
Copy link
Copy Markdown
Collaborator

@V-FEXrt V-FEXrt commented Apr 20, 2026

This is the final test change PR going into the initial preview build. We'll continue to add tests after the build that can be pulled from main or the preview branch.

In this PR are the following changes

  • OuterProduct smoke test was removed as it requires an OuterProductOptimal Layout, which requires a bit more test harness work to verify. Instead of delaying the preview for it we are punting the test for now. It will quickly be ready after the preview release
  • Groupshared operations require that stride be the number of "row elements" previously we were setting the "row bytes". This has been fixed
  • The 2D->1D index calculation was incorrect but hidden by the fact that all test matrices are square. This has been fixed

@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented Apr 20, 2026

✅ With the latest revision this PR passed the C/C++ code formatter.

Comment thread tools/clang/unittests/HLSLExec/LinAlgTests.cpp
Comment thread tools/clang/unittests/HLSLExec/LinAlgTests.cpp Outdated
Comment thread tools/clang/unittests/HLSLExec/LinAlgTests.cpp Outdated
Copy link
Copy Markdown
Contributor

@tex3d tex3d left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it would be better to always pass STRIDE in bytes (and side chat brought up OFFSET, so same issue), then divide STRIDE and OFFSET by ELEM_SIZE for any groupshared array based operations. That's assuming ELEM_SIZE is represents the native HLSL scalar size, which is different than the element size for packed elements!

@V-FEXrt V-FEXrt merged commit c763461 into microsoft:main Apr 21, 2026
12 checks passed
@github-project-automation github-project-automation Bot moved this from New to Done in HLSL Roadmap Apr 21, 2026
@xiaolin-ji
Copy link
Copy Markdown

Just to be clear, so for groupshared operation, the input offset to load&store is element count, not the byte offset?

For coordToByteOffset(), since coord.xy is (row, column), it should change to (coord.x * N_DIM + coord.y) * ELEM_SIZE for row major offset calculation.

@V-FEXrt
Copy link
Copy Markdown
Collaborator Author

V-FEXrt commented Apr 22, 2026

Just to be clear, so for groupshared operation, the input offset to load&store is element count, not the byte offset?

Yes! The spec needs to be clarified on this point. I'll be making that change after preview processes calm down

For coordToByteOffset(), since coord.xy is (row, column), it should change to (coord.x * N_DIM + coord.y) * ELEM_SIZE for row major offset calculation.

Per the other thread, I'm still not sure we agree on this point. I am happy to revisit it post-preview though just to really confirm. Since the smoke test matrices are all square this shouldn't effect the current tests even if it's wrong as is. We'll be adding non-square tests very soon so we'll be forced to get it right at that point regardless

@xiaolin-ji
Copy link
Copy Markdown

Just to be clear, so for groupshared operation, the input offset to load&store is element count, not the byte offset?

Yes! The spec needs to be clarified on this point. I'll be making that change after preview processes calm down

For coordToByteOffset(), since coord.xy is (row, column), it should change to (coord.x * N_DIM + coord.y) * ELEM_SIZE for row major offset calculation.

Per the other thread, I'm still not sure we agree on this point. I am happy to revisit it post-preview though just to really confirm. Since the smoke test matrices are all square this shouldn't effect the current tests even if it's wrong as is. We'll be adding non-square tests very soon so we'll be forced to get it right at that point regardless

thanks confirm the offset to group share operation.
For coordToByteOffset(), I just leave the comment at another thread, yes, N_DIM and M_DIM is same, but actually, we need spec to be clear, current coord.x is coordinate within the row or just row index.
if coord.x is coordinate within the row, and coord.y is coordinate within the column, then it is (coord.y * DIM + coord.x) * ELEM_SIZE
if coord.x is row and coord.y is column, then it should be (coord.x * DIM + coord.y) * ELEM_SIZE

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

4 participants