Fix test breakage in shaderc_util_compiler - Githubissues

google / shaderc

A collection of tools, libraries, and tests for Vulkan shader compilation.

Other

1.82k stars 353 forks source link

Fix test breakage in shaderc_util_compiler #612

Open zoddicus opened 5 years ago

zoddicus commented 5 years ago

CompilerTest.HlslFunctionality1Enabled in this test suite is failing, because the third_party/spirv-tools has not been rolled to pick up the SPIR-V 1.4 changes yet, but the test expectations were updated in #608

Specifically OpDecorateStringGOOGLE was changed to OpDecorateString.

dneto0 commented 5 years ago

Rolling glslang, spirv-tools, spirv-headers via #614

dneto0 commented 5 years ago

When rolling forward SPIRV-Tools, SPIRV-Headers, and Glslang, that fails.

When I also roll forward SPIRV-Cross, I get a bunch of failures, including validation failures showing SPIR-V 1.4 modules failing to validate for Vulkan 1.1. Many of those I can fix by forcing vulkan1.1 target environment for .asm. tests.

I still have a few left.

dneto0 commented 5 years ago

One of the errors looks like a bad test case in SPIRV-Cross: https://github.com/KhronosGroup/SPIRV-Cross/issues/967

fjhenigman commented 5 years ago

Did you run the spirv-cross tests (the actual spirv-cross tests, not the shaderc/spvc/spirv-cross tests)? It seems unlikely (though anything is possible) that there's a spirv-cross rev where their tests don't pass. If it works in spirv-cross it should work in shaderc/spvc/spirv-cross. If not then it's a good bet https://github.com/google/shaderc/blob/master/spvc/test/run_spirv_cross_tests.py needs updating to correctly mimic the behavior of https://github.com/KhronosGroup/SPIRV-Cross/blob/master/test_shaders.py

dneto0 commented 5 years ago

Good point. I just ran the SPIRV-Cross tests, when it's built with its own older versions of glslang, spirv-tools, and spirv-headers. They pass, with a small handful of unrelated warnings (array of arrays, and precision).

The changelog on test_shaders.py is pretty slim lately and does not seem relevant. So far the two issues @zoddicus and I have identified are independent of SPIRV-Cross passing its own tests. That is,

the bad test case I identified above is legitimately invalid GLSL (with the extension) and Glslang is rightly rejecting it.
When assembling .asm. cases, we should be explicitly specifying vulkan1.1 target environment (to get SPIR-V 1.3, rather than SPIR-V 1.4)

dneto0 commented 5 years ago

One more bad SPIRV-Cross test case https://github.com/KhronosGroup/SPIRV-Cross/blob/master/shaders/amd/shader_ballot_nonuniform_invocations.invalid.comp This exercises a bug I filed https://github.com/KhronosGroup/glslang/issues/1763 I suspect it's really a bug in SPV_AMD_shader_ballot and we'll end up relaxing the validation rules by saying SPV_AMD_shader_ballot enables the Groups capability.

dneto0 commented 5 years ago

The last failure is a legitimate validation failure:

WARNING: spirv-cross/shaders/flatten/multi-dimensional.desktop.invalid.flatten_dim.frag:4: '[][]' : Generating SPIR-V array-of-arrays, but Vulkan only supports single array level for this resource

validation failed: UniformConstant OpVariable '57[%uTextures]' has illegal type. From Vulkan spec, section 14.5.2: Variables identified with the UniformConstant storage class are used only as handles to refer to opaque resources. Such variables must be typed as OpTypeImage, OpTypeSampler, OpTypeSampledImage, OpTypeAccelerationStructureNV, or an array of one of these types. %uTextures = OpVariable %_ptr_UniformConstantarrarr__arr_52_uint_1_uint_3_uint_2 UniformConstant

This is a new validation check and it's legitimate. Really, Glslang should not be generating such code, but should be flattening the array. (I bet this is known, but should check.)