Skip to content

Add DXIL REQUIRES to certain tests that require a minimum validation versoin #7408

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 3 commits into from
May 1, 2025
Merged

Add DXIL REQUIRES to certain tests that require a minimum validation versoin #7408

merged 3 commits into from
May 1, 2025

Conversation

bob80905
Copy link
Collaborator

@bob80905 bob80905 commented Apr 30, 2025 •
edited
Loading

Internal testing using older validators reveals some failures on tests that were intended to be run on newer validators. This PR changes the tests to require a minimum validation version to run.
damyanp
damyanp approved these changes May 1, 2025
View reviewed changes
Copy link
Member

@damyanp damyanp left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please file a follow up task for us to look at these tests again.

We discussed offline - the tests that needed this change weren't expecting to run against the validator that was just built, these are tests that require an older validator to pass.
tex3d
tex3d requested changes May 1, 2025
View reviewed changes
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.

I don't think any of the added -select-validator external should have been necessary. I think this is an indication of a bug somewhere else.

On the change to require dxil-1-9 on a couple new tests, this is going to keep happening. I recommended that we place these tests under a subdirectory which blocks for specific validator version. That should make it easier to do the right thing in the first place.
tools/clang/unittests/HLSL/CompilerTest.cpp Outdated
CreateBlobFromText("float4 main() : SV_Target { return 0; }", &pSource);

LPCWSTR Args[2] = {OptLevel, L"/Odump"};
LPCWSTR Args[3] = {OptLevel, L"-select-validator external", L"/Odump"};
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/Odump should not run validation. I don't understand why this option would be necessary.
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can confirm that this change does fix this test failure. Previously there was a failure when running RunOptimizer.
tools/clang/test/LitDXILValidation/load-store-validation.ll
@@ -1,3 +1,4 @@
; REQUIRES: dxil-1-9
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should start a convention where we create sub-folders under LitDXILValidation for specific validator version requirements. All new tests would go into the latest folder, and we would have a lit.local.cfg that implements this requirement, so that none of the tests are required to individually state this requirement.
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

#7413
tools/clang/unittests/HLSL/DxilModuleTest.cpp Outdated
VerifyValidatorVersionMatches(L"ps_6_2", {});
VerifyValidatorVersionMatches(L"lib_6_3", {});
VerifyValidatorVersionMatches(L"ps_6_2", {L"-select-validator external"});
VerifyValidatorVersionMatches(L"lib_6_3", {L"-select-validator external"});
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't understand why we need any of this for any of these tests. If these tests fail without this added command line option, I think we have a bug. We should fix that bug, not add this option to the test.
@github-project-automation github-project-automation bot moved this from New to In progress in HLSL Roadmap May 1, 2025
@tex3d
Copy link
Contributor

tex3d commented May 1, 2025 •
edited
Loading

I suspect the real problem is that this code was not changed when we defaulted to using the internal validator:

DirectXShaderCompiler/tools/clang/tools/dxcompiler/dxcapi.cpp

Lines 90 to 94 in 6d67e4a

if (DxilLibIsEnabled()) {
hr = DxilLibCreateInstance(rclsid, riid, (IUnknown **)ppv);
} else {
hr = CreateDxcValidator(riid, ppv);
}

This is code used by dxcompiler.dll to pass through any IDxcValidator creation to an external validator (DXIL.dll) if one is present. We don't want to do that anymore, since we have a perfectly good signing validator in dxcompiler.dll now.

This can cause test failures because compilation is performed with the internal validator, which sets the validator version metadata to the latest version, then subsequent validation is performed on a validator created through the DxcCreateInstance API, which goes through this code path and selects the external validator that's present during the test run. That validator cannot validate the IR that has a newer validator version, so it fails.

This should be the fix:

diff --git a/tools/clang/tools/dxcompiler/dxcapi.cpp b/tools/clang/tools/dxcompiler/dxcapi.cpp index a6a877cba..ab2cf1f40 100644 --- a/tools/clang/tools/dxcompiler/dxcapi.cpp +++ b/tools/clang/tools/dxcompiler/dxcapi.cpp @@ -87,11 +87,7 @@ static HRESULT ThreadMallocDxcCreateInstance(REFCLSID rclsid, REFIID riid, } else if (IsEqualCLSID(rclsid, CLSID_DxcUtils)) { hr = CreateDxcUtils(riid, ppv); } else if (IsEqualCLSID(rclsid, CLSID_DxcValidator)) { - if (DxilLibIsEnabled()) { - hr = DxilLibCreateInstance(rclsid, riid, (IUnknown **)ppv); - } else { - hr = CreateDxcValidator(riid, ppv); - } + hr = CreateDxcValidator(riid, ppv); } else if (IsEqualCLSID(rclsid, CLSID_DxcAssembler)) { hr = CreateDxcAssembler(riid, ppv); } else if (IsEqualCLSID(rclsid, CLSID_DxcOptimizer)) {
@bob80905 bob80905 mentioned this pull request May 1, 2025
Closed
@bob80905 bob80905 changed the title Add external validation option explicitly to certain tests Add DXIL REQUIRES to certain tests that require a minimum validation versoin May 1, 2025
@bob80905 bob80905 merged commit 11e2895 into microsoft:main May 1, 2025
14 checks passed
@github-project-automation github-project-automation bot moved this from In progress to Done in HLSL Roadmap May 1, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

3 participants

@bob80905 @tex3d @damyanp