Skip to content

Update LongVector Execution tests to now use XML #7393

Merged
alsepkow merged 46 commits intomicrosoft:staging-sm6.9from
alsepkow:user/alsepkow/LongVector_Exec_XML
Jun 5, 2025
Merged

Update LongVector Execution tests to now use XML #7393
alsepkow merged 46 commits intomicrosoft:staging-sm6.9from
alsepkow:user/alsepkow/LongVector_Exec_XML

Conversation

@alsepkow
Copy link
Copy Markdown
Contributor

@alsepkow alsepkow commented Apr 28, 2025

This change refactors the LongVector exec tests into their own files and swaps to use the XML and statically defined values for test input.

  1. Update the LongVector exec tests to use the ShaderOpArithTable.xml TAEF table file for test entry points. This aligns with existing HLK tests.
  2. Some light code cleanup.
  3. Hard coded value sets in LongVectorTestData.h. Value sets give us a simple way to generate larger arrays from a smaller set of statically defined values. At a later point we can add logic to produce value sets at build time in this same header by consuming from a YAML file used in the offload test suite.
  4. Move LongVector tests into their own LongVector.h, LongVector.tpp, and LongVector.cpp files.
  5. Add an HLSLExecTestUtils.h file to hold some common logic to facilitate re-factoring the LongVector tests into their own files.

This PR ended up growing larger than intended. Subsequent PRs will be smaller chunks.
There are some additional refactoring and clean-up changes coming but I did not want to continue to add to this PR.

@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented Apr 28, 2025

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

@damyanp
Copy link
Copy Markdown
Member

damyanp commented Apr 29, 2025

I'm guessing that the submodule changes aren't meant to be a part of this change?

@alsepkow
Copy link
Copy Markdown
Contributor Author

I'm guessing that the submodule changes aren't meant to be a part of this change?

They are not. Accidentally grabbed those when I added the clang-format. Will fix.

Comment thread tools/clang/unittests/HLSLExec/LongVectors.h Outdated
Comment thread tools/clang/unittests/HLSLExec/LongVectors.h Outdated
Comment thread tools/clang/unittests/HLSLExec/LongVectors.h Outdated
Comment thread tools/clang/unittests/HLSLExec/LongVectors.h Outdated
Comment thread tools/clang/unittests/HLSLExec/LongVectors.h Outdated
Comment thread tools/clang/unittests/HLSLExec/LongVectorTestData.h Outdated
Comment thread tools/clang/unittests/HLSLExec/ExecutionTest.cpp Outdated
LongVectorOpType_Clamp,
LongVectorOpType_Initialize,
LongVectorOpType_UnInitialized
enum LongVectorBinaryOpType {
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Just want to note here that for subsequent PR's that add more tests cases I'm going to split these enums up by category instead of unary vs binary.
HLSLOperators, trigonometric, math, etc. And then I'll also have some child classes that inherit from LongVectorTestConfig for these various categories to help split out the logic by category and avoid large switch statements.

Comment thread include/dxc/Test/HlslTestUtils.h
Comment thread include/dxc/Test/HlslTestUtils.h Outdated
Comment thread tools/clang/unittests/HLSLExec/ExecutionTest.cpp Outdated
Comment thread tools/clang/unittests/HLSLExec/ExecutionTest.cpp Outdated
Comment thread tools/clang/unittests/HLSLExec/ExecutionTest.cpp Outdated
Comment thread tools/clang/unittests/HLSLExec/ExecutionTest.cpp Outdated
Comment thread tools/clang/unittests/HLSLExec/LongVectors.h Outdated
Comment thread tools/clang/unittests/HLSLExec/LongVectors.h Outdated
Comment thread tools/clang/unittests/HLSLExec/LongVectors.h Outdated
Comment thread include/dxc/Test/HlslTestUtils.h Outdated
Comment thread include/dxc/Test/HlslTestUtils.h Outdated
Comment thread include/dxc/Test/HlslTestUtils.h Outdated
Comment thread tools/clang/unittests/HLSLExec/CMakeLists.txt
Comment thread tools/clang/unittests/HLSLExec/ExecutionTest.cpp Outdated
Comment thread tools/clang/unittests/HLSLExec/LongVectors.h
Copy link
Copy Markdown
Collaborator

@bob80905 bob80905 left a comment

Choose a reason for hiding this comment

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

Just some nits, take em or leave em

Comment thread tools/clang/unittests/HLSLExec/LongVectorTestData.h Outdated
Comment thread tools/clang/unittests/HLSLExec/LongVectors.h
Comment thread include/dxc/Test/HlslTestUtils.h Outdated
Comment thread tools/clang/unittests/HLSLExec/LongVectors.tpp Outdated
Comment thread utils/version/latest-release.json
Comment thread tools/clang/unittests/HLSLExec/ShaderOpArithTable.xml Outdated
Comment thread tools/clang/unittests/HLSLExec/LongVectors.tpp
Comment thread tools/clang/unittests/HLSLExec/LongVectors.h 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.

There are some things I think we should do when taking these changes to main:

  • Break up the changes into smaller parts: refactors of existing code could be reviewed and merged ahead of the code that depends on it, and make subsequent reviews much easier.
  • Make sure these changes will work with the HLK content porting script (understanding changes necessary there).
  • If there's a way to simplify the design to make it easier to understand, add, and maintain tests, take that into consideration.

@alsepkow alsepkow force-pushed the user/alsepkow/LongVector_Exec_XML branch from ee4a8ed to b5fdc60 Compare June 4, 2025 05:56
@alsepkow alsepkow merged commit 3d3df3f into microsoft:staging-sm6.9 Jun 5, 2025
12 checks passed
@github-project-automation github-project-automation Bot moved this from New to Done in HLSL Roadmap Jun 5, 2025
@alsepkow alsepkow deleted the user/alsepkow/LongVector_Exec_XML branch June 25, 2025 02:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Archived in project

Development

Successfully merging this pull request may close these issues.

4 participants