search

KhronosGroup / glslang

Khronos-reference front end for GLSL/ESSL, partial front end for HLSL, and a SPIR-V generator.
Other
2.9k stars 816 forks source link

Propose A Better Scheme for Id Allocation Specifically For The Testing Purpose #3512

Open qingyuanzNV opened 4 months ago

qingyuanzNV commented 4 months ago

It's great that we maintain a large repository of shaders in this project to ensure that glslang is composed with high quality code. However, the exact-match model in test result and the counting approach of id allocation strategy are making it difficult to reason about the test result change. Even a really simple change in glslang could lead to an id shift and all relevant test results may be edited entirely.

Could we add a new scheme of id allocation for testing only so that those ids are somewhat more stable, and the test result changes are easier to reason about? For example, we may let "id = opcode * 10000 + offset" so at least ids are stable across different opcodes?

arcady-lunarg commented 4 months ago

I think the concern about spurious diffs is a valid one, and it's been an issue for me as well when trying to review changes that add or remove some instructions at the beginning of a SPIR-V file and thus result in big diffs to test results where the actual change is really small and everything else is just changes in Ids. On the other hand, I don't think it's a great idea to maintain separate paths for Id allocation and technically what you propose goes against the SPIR-V spec recommendation to have the Id space be reasonably compact. There is a tool in SPIRV-Tools called spirv-diff, which tries to do structural diffing between SPIR-V files, however it's not currently used in our test harness, nor is it really directly usable because of the format the result files, which is not binaries nor is it the standard text format as used by spirv-as and spirv-dis. If you want to integrate this into the test harness somehow though, I think that would be a great patch and would make my life easier as a maintainer.

qingyuanzNV commented 4 months ago

To clarify, I'm proposing this scheme only for testing. Those modules shouldn't be generated in real world for end users. Perhaps this could also be done by separately maintained tools instead of directly in glslang.

spirv-diff is also a promising option. Though we have to come up with some approach to make is usable as obviously we can't store diffs as golden files.

qingyuanzNV commented 4 days ago

Hi @arcady-lunarg, I did some experiments today, including trying spirv-diff and remapping the id during disassembly.

First, I manually tried to dump some shader from the glslangtest and spirv-diff doesn't work well. The diff is even more complicated somehow and it crashes sometimes. Aside from changing how we allocate ids in glslang and introducing complexity in glslang code, what about remapping/stabilizing those ids while dumping the test disassembly? We already have this custom SPIR-V disassembly logic in SPIRV/disassembly.cpp, which we can take advantage of. I made a proof of concept change in https://github.com/KhronosGroup/glslang/compare/main...qingyuanzNV:glslang:proof_of_concept_test_id_remap I'm using another debug change I'm going to make as a testbed. Please refer to diffs of test output in "first commit" and "diff for remapped id". Although it's not perfect, it significantly improves the diff and give me confidence that I can reason about the change in the test result.