SPIRV-Tools is a toolset for working with SPIR-V, including a disassembler and validator. It is well-maintained and kept up to date with the evolving SPIR-V specification. The validator checks many validation rules described by the SPIR-V specification both for the core features and for extensions.
Proposal
This RFC proposes adding an external test dependency on SPIRV-Tools. This will allow us to more easily test the SPIR-V backend as we continue to develop it and catch certain classes of bugs much more easily. When the CMake flag LLVM_INCLUDE_SPIRV_TOOLS_TESTS is enabled, the project would be fetched from Github and built, linking the spirv-val (validator) and spirv-dis (disassembler) binaries to the runtime directory for usage in lit tests. An option would also be given to instead provide paths to prebuilt binaries via SPIRV_VAL and SPIRV_DIS variables.
Benefits
The benefits of adding SPIRV-Tools include:
Validation of test output: The new test tools will provide additional coverage for the SPIR-V backend by allowing us to easily expand new and existing tests to also run the SPIR-V validator on test output.
Test coverage of binary output: The disassembler will allow us to test the binaries generated from tests using FileCheck.
Reduced risk of regressions: The new tests will help to reduce the risk of regressions in the SPIR-V backend as we add additional complexity to support the Vulkan ecosystem.
Drawbacks
The main drawback of adding an external test dependency on SPIRV-Tools is the additional build complexity that it would introduce and maintenance of build rules that depend on an external project. However, we believe that the benefits of the proposal outweigh the drawbacks as it would be limited to builds explicitly enabling LLVM_INCLUDE_SPIRV_TOOLS_TESTS. When the flag is disabled, SPIR-V backend tests that do not have a REQUIRES: spirv-tools constraint would still be run.
Conclusion
We believe that adding an external test dependency on SPIRV-Tools to LLVM is a worthwhile trade-off to aid in the development of the SPIR-V backend and maintain and improve its quality going forward.
Thank you Natalie for posting this RFC and preparing the draft pull request!
Just to add a bit more context for other commenters:
SPIR-V Tools would just help in providing additional checks on top of already self-contained LIT tests. These checks would be very beneficial for us, since many of the issues that SPIR-V Validator is able to find we could only discover either by manually running and comparing validation results or when running full runtime testing. In the past, due to time constraints and permissibility of SPIR-V consumers, validation regressions were often unnoticed.
As we are adding SPIR-V extension and Vulkan support, validation plays a more significant role.
SPIR-V Disassembler would also serve as a verifier of binary compatibility between the backend and SPIR-V consumers. Those issues are not common at all, but it is better to be safe.
With SPIR-V being an IR, this means if there is one buildbot that has these tools installed and runs the tests, you have enough coverage. Is that correct?
For example, you wouldn’t, need an Arm and an Intel bot, as the SPIR-V tools should behave the same way on all platforms.
Do you have an existing buildbot in mind or will a new one be added?
Folks will also have to be able to reproduce these failures, which sounds quite easy once you know how. If there is a SPIR-V status page somewhere in llvm already maybe add a note there that there is this category of tests that need the extra tools.
If a new buildbot is going to be added for this only, you could put a note in the worker description. The sanitizer bots do something like this.
With SPIR-V being an IR, this means if there is one buildbot that has these tools installed and runs the tests, you have enough coverage. Is that correct?
Correct.
Do you have an existing buildbot in mind or will a new one be added?
SPIR-V backend is not built by default (it is still an experimental target) none of the buildbots executing check-all will run SPIR-V LIT tests. This will eventually change. We do have our runtime testing buildbot and a status page that is yet to be reconfigured after LLVM’s switch to GitHub pull requests from Phabricator. This buildbot will run the LIT tests with SPIR-V Tools and allow us to give users some more insights on how these failures can be reproduced and fixed.
I believe it is important that most of the tests can be executed without any external tools – at least if you ever want this target to become non-experimental, which means that everyone has to deal with the target, rather than only a small group of people who naturally use this additional tooling.
Basically, NVPTX uses normal tests, but additionally includes a RUN line of the form %if ptxas %{ llc < %s -march=nvptx64 -mcpu=sm_70 -mattr=+ptx60 | %ptxas-verify -arch=sm_70 %}. This means that if ptxas is available, the llc result will be checked against ptxas to ensure the emitted assembly verifies.
I expect that you could do something similar for the SPIRV case. This has the big advantage that the tests all run and are meaningful even without the external tool. It only provides additional verification on top of the usual test.
I agree with Nikita’s point. I think it’s fine to have tests or checks in the LLVM test suite that rely on optional tools not available to the average developer, but we must ensure that developers without spirv-tools have some level of testing that prevents them from breaking the SPIR-V backend.
The NVPTX solution seems like one good way to do that, but any solution supporting that goal seems reasonable.
I also want to point out that spirv-val and spirv-dis seem like really well-behaved tools that fit well into the LLVM testing ecosystem, in a way that larger system integration tests, like with a debugger, for example, would not. We have other suites (cross-project-tests) for larger integration tests that produce less immediately actionable results on failure.
Thanks @nikic! I agree – I was limiting the dependency to only some test files with REQUIRES: spriv-tools so other SPIR-V backend tests would run when it wasn’t available but I didn’t realize that we could make it more granular to just a RUN line with %if. I’ll add this to the PR and make sure they run as expected with and without LLVM_INCLUDE_SPIRV_TOOLS_TESTS enabled.
Also, I noticed that the description said “SPIR-V tests now have a dependency on the spirv-dis and spirv-val targets, and are only built when the LLVM_INCLUDE_SPIRV_TOOLS_TESTS cmake variable is set.” which was incorrect and conflicted with this RFC. Sorry if that caused any confusion. I forgot to update the commit message after switching to the spirv-tools feature flag to guard individual tests.