Add back compat lit test suite, and download older dxil.dll's #7705
Conversation
|
IMO it'd be good to combine this with the PR that actually lights up this testing. Then we can see that everything is working together as planned. |
bob80905
force-pushed
the
download_dxc_release_zips
branch
from
7ada634 to
de8f1a0
Compare
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
bob80905
force-pushed
the
download_dxc_release_zips
branch
from
c5ce73c to
29a74c8
Compare
tools/clang/test/CMakeLists.txt
Outdated
| else() | ||
| # Single-config generators (Ninja, Makefiles) | ||
| # Use pointer size to guess x86 vs x64 | ||
| if(CMAKE_SIZEOF_VOID_P EQUAL 8) |
tools/clang/test/lit.cfg
Outdated
| print("PARAMS seen in top level config:", lit_config.params) | ||
|
|
||
|
|
There was a problem hiding this comment.
Is this left-over debugging code, or do you think this should be here long term?
| dxc::DxcDllExtValidationLoader ExtSupport; | ||
| ExtSupport.Initialize(); | ||
| std::string dllPath = ExtSupport.GetDxilDllPath(); | ||
| VERIFY_IS_TRUE(dllPath.empty()); |
There was a problem hiding this comment.
What does it mean that dllPath is empty? I'm trying to understand what this test is testing.
There was a problem hiding this comment.
This test is going to be out of scope for now. I'm going to focus on getting check-clang to work with older external validators.
But the point was, this test would be run when an older validator was set in the env var, and so we expect initialization to pull the env var's value and the loader object should have a stored dll path (it shouldn't be empty since it was read from the env var). It would only be empty if the env var was never set, thus proving that the infrastructure correctly set the environment.
This shouldn't be tested quite yet since it seems like a whole separate ordeal to add taef-based tests. We'll probably need to adjust the standard taef-test filter so that the original check-clang-taef_exec target runs on .ll / .hlsl / .test, AND .cpp files that are NOT the back-compat validation tests, otherwise they would fail since the environment wasn't set up.
It's a separate problem for now.
| @@ -196,6 +196,20 @@ std::string DisassembleProgram(dxc::DxCompilerDllLoader &dllSupport, | |||
| return BlobToUtf8(pDisassembly); | |||
| } | |||
|
|
|||
| std::string DisassembleProgram(dxc::DxcDllExtValidationLoader &dllSupport, | |||
There was a problem hiding this comment.
I'm having trouble connecting the dots here as to why the changes involving DisassembleProgram are related to the rest of these changes?
There was a problem hiding this comment.
Yes, this was when I had thought I could do taef tests and modify the loaders all in one PR, I do think this will be needed, but later :)
There was a problem hiding this comment.
Should we keep this in this change or should it be pulled out to the later one?
bob80905
changed the title
bob80905
changed the title
| @@ -196,6 +196,20 @@ std::string DisassembleProgram(dxc::DxCompilerDllLoader &dllSupport, | |||
| return BlobToUtf8(pDisassembly); | |||
| } | |||
|
|
|||
| std::string DisassembleProgram(dxc::DxcDllExtValidationLoader &dllSupport, | |||
There was a problem hiding this comment.
Should we keep this in this change or should it be pulled out to the later one?
azure-pipelines.yml
Outdated
| @@ -13,7 +13,8 @@ stages: | |||
| - stage: Build | |||
| jobs: | |||
| - job: Windows | |||
| timeoutInMinutes: 120 | |||
| # Including back-compat tests with 3 older validators, about 15 minutes each | |||
There was a problem hiding this comment.
TBH I find this comment a bit unnecessary. It justifies the change, but for a comment itself why doesn't it say something about how long all of the other things are taking? Mentioning the reason for the diff in the commit is worthwhile, but otherwise this starts begging questions. Like: why isn't it 45 minutes if it's doing 3 things that are 15 mins each?
There was a problem hiding this comment.
Yeah that's fair, I'll remove the comment
This PR adds new lit test suites, that build by downloading and unzipping 5 release packages. The release packages are a selection of historical packages that will be useful in later testing for validation back-compatibility. However, this data can also be used for other testing, and doesn't necessarily need to be restricted to older packages. For now, 5 releases have been included, including the latest 3 releases at the time of this writing.
This PR then executes these test suites with an environment variable that points to the dxil.dll that was just downloaded, so that in the future, DxcDllExtValLoader can grab the dxil.dll path and initialize appropriately.
This PR effectively sets up a test suite and an environment so that back compat work is possible.
Adds some final touches on @damyanp's work.
Fixes #7707