Long Vector Execution Tests: Merge unary and binary op tests to main#7549
Conversation
Remove half and bool types for less code to review right now
tex3d
left a comment
tex3d
left a comment
There was a problem hiding this comment.
Just one minor comment regarding double denorms that could probably be addressed in a follow up.
| return std::isnan(Ref); | ||
| } | ||
|
|
||
| if (Mode == hlsl::DXIL::Float32DenormMode::Any) { |
There was a problem hiding this comment.
Just FYI: denorms are required to be preserved for double-precision DXIL operations. So either this should default to Float32DenormMode::Preserve, or we should just remove this branch.
There was a problem hiding this comment.
I can address in this PR. Conflicts is this file will be trivial in the diff for merging the rest.
bob80905
left a comment
bob80905
left a comment
There was a problem hiding this comment.
There are a couple files that don't have a new line at EOF you might consider adding.
| if (!Initialized) { | ||
| Initialized = true; | ||
|
|
||
| HMODULE Runtime = LoadLibraryW(L"d3d12.dll"); |
There was a problem hiding this comment.
Where is this library freed?
There was a problem hiding this comment.
Good question, I wondered the same. This setup code is copied from
ExecutionTest.cpp and has the note below to not call FreeLibrary on the handle.
Which I now realize has a typo from the naming update. I'll do a little digging
and report back - this is worth understanding to make sure we aren't leaking the
library handle.
There was a problem hiding this comment.
There is no explicit FreeLibrary anywhere in this code. The library is
implicilty freed on process exit. This isn't inherently bad....but this is
generally considered bad practice. I think the original author was probably just
lazy. I dont see a good reason for why we shouldn't store this handle as a
member variable and call FreeLibrary on it in cleanup. I'll create an issue to
address that in a subsequent PR.
There was a problem hiding this comment.
Well, I was trying the 'github pull requests' plugin for vscode out. Looks like its reply to comments just submits new comments. Pasting them here.
Good question, I wondered the same. This setup code is copied from
ExecutionTest.cpp and has the note below to not call FreeLibrary on the handle.
Which I now realize has a typo from the naming update. I'll do a little digging
and report back - this is worth understanding to make sure we aren't leaking the
library handle.
alsepkow
deleted the
user/alsepkow/LongVector_Main_Initial_RemoveBoolAndHalf
branch
3 participants
Summary
Adds infrastructure for long vector execution tests. This code and additional test cases were already added to the staging-sm6.9 branch. This is the second of several PRs to bring these changes into main. That being said, reviews of this code should treat it as brand new. Resolves #7545
Includes:
LongVector::OpTestinLongVectors.h/cpp, still part of theExecHLSLTests.dllbinary.ShaderOpArith.xmlto leverage the existing exec test framework for shader compilation and execution.LongVectorOpTable.xmldefining long vector test cases.LongVectorTestData.hfor statically defined input values, includingHLSLHalf_tandHLSLBool_t. This avoids duplicating values across test cases.Template Handling
To support template instantiation across translation units,
LongVectors.tppcontains full template definitions included byLongVectors.h. These were originally required when tests lived inExecutionTests.cpp. Now that the tests are isolated, the plan is to move the definitions back intoLongVectors.cppafter merging the long vector tests fromstaging-sm6.9to simplify the manual merge.Utilities
HlslTestUtils.hincludes minor updates to support the new test scenarios.