-
Notifications
You must be signed in to change notification settings - Fork 778
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
Update LongVector Execution tests to now use XML #7393
Conversation
✅ With the latest revision this PR passed the C/C++ code formatter. |
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 { |
There was a problem hiding this comment.
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.
There was a problem hiding this 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
There was a problem hiding this 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.
…little cleanup and should update formatting given that its new files now
ee4a8ed
to
b5fdc60
Compare
This change refactors the LongVector exec tests into their own files and swaps to use the XML and statically defined values for test input.
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.