Skip to content

Update LongVector Execution tests to now use XML #7393

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged

Conversation

alsepkow
Copy link
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.

Copy link
Contributor

github-actions bot commented Apr 28, 2025

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

@damyanp
Copy link
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
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.

LongVectorOpType_Clamp,
LongVectorOpType_Initialize,
LongVectorOpType_UnInitialized
enum LongVectorBinaryOpType {
Copy link
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.

Copy link
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

Copy link
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
Status: Done
Development

Successfully merging this pull request may close these issues.

4 participants