Fix auto-dxilver logic in FileCheckerTest #6368
Conversation
In FileCheckerTest.cpp, the -T target of a %dxc part is checked for compatibility. However, if you use -Vd, it skips the version check for the validator. This isn't quite right since the validator version will be set from the attached validator, unless you explicitly set -validator-version or use -select-validator internal. Also, if -validator-version is explicitly set, we should check the validator against that version, not just the one based on shader model. This change fixes the initial check logic to only skip validator check when using -select-validator internal or -validator-version. It also checks against an explicitly requested -validator-version when an external validator will be used.
There was a problem hiding this comment.
I believe that the lack of support for loading external test validators prevents us from committing this with any testing really, but I think we have some internal testing that will allow it. Is that correct? Will there be testing there? The state of #5952 doesn't currently allow lower version validators, but it could. Do you think we should polish that off and get it in for the sake of this?
|
Does lit FileCheck with things like have the same issue? |
Seems there should be some test coverage for this. If we can't test is then that presumably means that there's no observable change from this at all? |
As you say, #5952 wouldn't help in its current state. I'm not sure how we would add testing for lower versions, other than perhaps building multiple validators and versioning them in this way. That seems like overkill for testing this mechanism in our FileCheckerTest. I could add a test that doesn't necessarily trip this scenario in CI testing here, but would verify that this mechanism works upon integration testing internally. Would that be acceptable instead?
There's an observable change in our internal downlevel validator coverage upon integration. I can add a test that would trigger a failure there without the fix.
No, this does not have the same issue. I will add a test that would fail our internal integration testing. Hopefully that is sufficient. |
In FileCheckerTest.cpp, the -T target of a %dxc part is checked for compatibility. However, if you use -Vd, it skips the version check for the validator. This isn't quite right since the validator version will be set from the attached validator, unless you explicitly set -validator-version or use -select-validator internal. Also, if -validator-version is explicitly set, we should check the validator against that version, not just the one based on shader model.
This change fixes the initial check logic to only skip validator check when using -select-validator internal or -validator-version. It also checks against an explicitly requested -validator-version when an external validator will be used.
Fixes #6367