Conversation
* Descriptor maps for both online and offline compiles use embedded reflection information * replaced old parsers with a single unified parser * If VK_KHR_non_semantic_info is not supported, the reflection is stripped from the binary before creating a shader module * Descriptor maps are no longer written to saved binaries
* Add clspv include dirs for both modes * Don't pass -desriptormap to clspv anymore
* update spirv-llvm-translator deps
* Update clspv * Update SPIRV-LLVM-Translator (also point at KhronosGroup)
kpet
left a comment
There was a problem hiding this comment.
This looks generally great! A few observations:
- Changing the interface between clspv and clvk is now harder. One has to change the non semantic instruction set specification in the registry first, generate updated headers, update clspv and finally clvk. Changes are not that frequent so I think it's worth trading the added complexity for a cleaner interface.
- I've pulled this change locally and run a CTS sweep using an offline build.
test_compileris crashing andwrite_image_{float,char,uchar}have regressed. - We'll probably want to do more renaming/refactoring but that can happen after this change.
- Going forward we'll probably want clvk binaries to just be SPIR-V modules. I can't think of anything that couldn't be added to the SPIR-V at the moment. This would make offline compilation much easier as one could use clspv directly.
| m_dmaps[entry.kernel_arg_data.kernel_name].push_back(arg); | ||
| spvtools::Optimizer opt(m_target_env); | ||
| opt.SetMessageConsumer(consumer); | ||
| opt.RegisterPass(spvtools::CreateStripReflectInfoPass()); |
There was a problem hiding this comment.
Maybe the pass factory should have had "clspv" in the name to be consistent with the instruction set name.
There was a problem hiding this comment.
The reflection stripping pass existed before my instruction set and handles non-semantic instructions generally and also the HLSL reflection instructions and decorations.
There was a problem hiding this comment.
Ok, that's a bit counter-intuitive. I'm wondering whether generic stripping of all non-semantic instructions shouldn't be a separate pass. I'm not familiar enough with the HLSL bits to have an informed opinion though.
|
I'll take a look at the CTS failures.
Agreed, but I wanted to keep this change as minimal as reasonably possible. |
|
I believe the SPIRV-Tools linker/optimizer is having problems with the extended instructions. Need to investigate how to fix that. |
SPIRV-Tools optimizer representation is reordering the module and producing an invalid binary. |
* Update SPIRV-Tools to include fix handling of non-semantic instructions in the optimizer * Improving naming * Use extension name macro instead of raw string
|
The last push fixes the compiler conformance test crash. All those tests match the checked in results. With that and the sampler fix, CTS should be good now. |
|
All the usual CTS tests are now passing. All looking good, thanks! |
Change clvk to use clspv's embedded reflection in the SPIR-V binary.