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.
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.
| @@ -3,6 +3,8 @@ | |||
| find_package(TAEF REQUIRED) | |||
| find_package(D3D12 REQUIRED) # Used for ExecutionTest.cpp. | |||
|
|
|||
| set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} /bigobj") | |||
There was a problem hiding this comment.
I don't see why we couldn't break things into multiple cpp files that we could copy over for the HLK test (like we do for ShaderOpTest.cpp). This ExecutionTest.cpp has gotten a bit out of hand!
There was a problem hiding this comment.
I'll take a look into doing that. Originally this code was more coupled with other helper code in ExecutionTest.cpp.
A few thoughts come to mind though:
- How much time to we want to spend trying to clean this up right now vs getting long vector exec tests in.
- We might do a refactoring/re-writing of a bunch of this in the not-too-distant future.
- "We'll do this later" often just turns into never 😔.
But I'll still take a closer look and see what I could easily break up. I think it will make reviews easier.
There was a problem hiding this comment.
I'd need to factor out some of the ShaderOpTest* code starting on line 3531 with the ShaderOpTestResultStruct. Seems reasonable to put that into ShaderOpTest.h/cpp. And the TableParameter/TableParameterHandler stuff would need to be factored out. That could probably also go in its own appropriately named header/cpp file.
Both seem like reasonable updates I could make to help clean this file up a little. Think its worth doing that?
| BinaryOpType_EnumValueCount | ||
| }; | ||
|
|
||
| static const LongVectorOpTypeStringToEnumValue BinaryOpTypeStringToEnumMap[] = { |
There was a problem hiding this comment.
Couldn't these be a std::map?
There was a problem hiding this comment.
I wanted to have a static assert on the size to have a compile error if you forget to add to this enum value when adding an *OpType. Even declaring the std::map as static const still wouldn't allow a static assert on the size.
Open to ideas if you think some other pattern is cleaner though.
…little cleanup and should update formatting given that its new files now
This change is mainly focused on swapping to use the XML and statically defined values. Opting to stage this for review in smaller chunk while I iterate on adding the additional test cases.