From cf43e661251f7797e469a93fa0ecd3e66980eca7 Mon Sep 17 00:00:00 2001 From: steve-lunarg Date: Thu, 22 Sep 2016 14:35:23 -0600 Subject: [PATCH] Fix defects in uniform array flattening Fix for two defects as follows: - The IO mapping traverser was not setting inVisit, and would skip some AST nodes. Depending on the order of nodes, this could have prevented the binding from showing up in the generated SPIR-V. - If a uniform array was flattened, each of the flattened scalars from the array is still a (now-scalar) uniform. It was being converted to a temporary. --- Test/baseResults/hlsl.array.flatten.frag.out | 10 ++-- .../spv.register.autoassign-2.frag.out | 57 +++++++++++++++++++ Test/spv.register.autoassign-2.frag | 15 +++++ glslang/MachineIndependent/LiveTraverser.h | 4 +- glslang/MachineIndependent/iomapper.cpp | 2 +- gtests/Spv.FromFile.cpp | 9 ++- gtests/TestFixture.h | 10 +++- hlsl/hlslParseHelper.cpp | 21 ++++--- 8 files changed, 109 insertions(+), 19 deletions(-) create mode 100644 Test/baseResults/spv.register.autoassign-2.frag.out create mode 100644 Test/spv.register.autoassign-2.frag diff --git a/Test/baseResults/hlsl.array.flatten.frag.out b/Test/baseResults/hlsl.array.flatten.frag.out index a3752251..eedd698f 100644 --- a/Test/baseResults/hlsl.array.flatten.frag.out +++ b/Test/baseResults/hlsl.array.flatten.frag.out @@ -8,8 +8,8 @@ gl_FragCoord origin is upper left 0:18 Branch: Return with expression 0:18 texture (global 4-component vector of float) 0:18 Construct combined texture-sampler (temp sampler1D) -0:? 'g_tex[1]' (temp texture1D) -0:? 'g_samp[1]' (temp sampler) +0:? 'g_tex[1]' (uniform texture1D) +0:? 'g_samp[1]' (uniform sampler) 0:18 Constant: 0:18 0.200000 0:22 Function Definition: TestFn2(t11[3];p1[3]; (global 4-component vector of float) @@ -197,8 +197,8 @@ gl_FragCoord origin is upper left 0:18 Branch: Return with expression 0:18 texture (global 4-component vector of float) 0:18 Construct combined texture-sampler (temp sampler1D) -0:? 'g_tex[1]' (temp texture1D) -0:? 'g_samp[1]' (temp sampler) +0:? 'g_tex[1]' (uniform texture1D) +0:? 'g_samp[1]' (uniform sampler) 0:18 Constant: 0:18 0.200000 0:22 Function Definition: TestFn2(t11[3];p1[3]; (global 4-component vector of float) @@ -419,6 +419,8 @@ gl_FragCoord origin is upper left Name 125 "g_mats_explicit[1]" Name 126 "g_mats_explicit[2]" Name 127 "g_mats_explicit[3]" + Decorate 36(g_tex[1]) DescriptorSet 0 + Decorate 39(g_samp[1]) DescriptorSet 0 Decorate 57(g_samp[0]) DescriptorSet 0 Decorate 62(g_samp[2]) DescriptorSet 0 Decorate 66(g_tex[0]) DescriptorSet 0 diff --git a/Test/baseResults/spv.register.autoassign-2.frag.out b/Test/baseResults/spv.register.autoassign-2.frag.out new file mode 100644 index 00000000..038c7f0e --- /dev/null +++ b/Test/baseResults/spv.register.autoassign-2.frag.out @@ -0,0 +1,57 @@ +spv.register.autoassign-2.frag + +Linked fragment stage: + + +// Module Version 10000 +// Generated by (magic number): 80001 +// Id's are bound by 30 + + Capability Shader + 1: ExtInstImport "GLSL.std.450" + MemoryModel Logical GLSL450 + EntryPoint Fragment 4 "main" 9 + ExecutionMode 4 OriginUpperLeft + Name 4 "main" + Name 9 "Color" + Name 12 "g_tScene[0]" + Name 16 "g_tSamp" + Name 24 "g_tScene[1]" + Decorate 9(Color) Location 0 + Decorate 12(g_tScene[0]) DescriptorSet 0 + Decorate 12(g_tScene[0]) Binding 10 + Decorate 16(g_tSamp) DescriptorSet 0 + Decorate 16(g_tSamp) Binding 5 + Decorate 24(g_tScene[1]) DescriptorSet 0 + Decorate 24(g_tScene[1]) Binding 11 + 2: TypeVoid + 3: TypeFunction 2 + 6: TypeFloat 32 + 7: TypeVector 6(float) 4 + 8: TypePointer Output 7(fvec4) + 9(Color): 8(ptr) Variable Output + 10: TypeImage 6(float) 2D sampled format:Unknown + 11: TypePointer UniformConstant 10 + 12(g_tScene[0]): 11(ptr) Variable UniformConstant + 14: TypeSampler + 15: TypePointer UniformConstant 14 + 16(g_tSamp): 15(ptr) Variable UniformConstant + 18: TypeSampledImage 10 + 20: TypeVector 6(float) 2 + 21: 6(float) Constant 1050253722 + 22: 20(fvec2) ConstantComposite 21 21 + 24(g_tScene[1]): 11(ptr) Variable UniformConstant + 4(main): 2 Function None 3 + 5: Label + 13: 10 Load 12(g_tScene[0]) + 17: 14 Load 16(g_tSamp) + 19: 18 SampledImage 13 17 + 23: 7(fvec4) ImageSampleImplicitLod 19 22 + 25: 10 Load 24(g_tScene[1]) + 26: 14 Load 16(g_tSamp) + 27: 18 SampledImage 25 26 + 28: 7(fvec4) ImageSampleImplicitLod 27 22 + 29: 7(fvec4) FAdd 23 28 + Store 9(Color) 29 + Return + FunctionEnd diff --git a/Test/spv.register.autoassign-2.frag b/Test/spv.register.autoassign-2.frag new file mode 100644 index 00000000..b943791f --- /dev/null +++ b/Test/spv.register.autoassign-2.frag @@ -0,0 +1,15 @@ + +SamplerState g_tSamp : register(s0); + +Texture2D g_tScene[2] : register(t0); + +struct PS_OUTPUT +{ + float4 Color : SV_Target0; +}; + +void main(out PS_OUTPUT psout) +{ + psout.Color = g_tScene[0].Sample(g_tSamp, 0.3) + + g_tScene[1].Sample(g_tSamp, 0.3); +} diff --git a/glslang/MachineIndependent/LiveTraverser.h b/glslang/MachineIndependent/LiveTraverser.h index 5a902049..286efe6c 100644 --- a/glslang/MachineIndependent/LiveTraverser.h +++ b/glslang/MachineIndependent/LiveTraverser.h @@ -56,7 +56,9 @@ namespace glslang { class TLiveTraverser : public TIntermTraverser { public: - TLiveTraverser(const TIntermediate& i, bool traverseAll = false) : + TLiveTraverser(const TIntermediate& i, bool traverseAll = false, + bool preVisit = true, bool inVisit = false, bool postVisit = false) : + TIntermTraverser(preVisit, inVisit, postVisit), intermediate(i), traverseAll(traverseAll) { } diff --git a/glslang/MachineIndependent/iomapper.cpp b/glslang/MachineIndependent/iomapper.cpp index d02fb5ea..22d97c29 100644 --- a/glslang/MachineIndependent/iomapper.cpp +++ b/glslang/MachineIndependent/iomapper.cpp @@ -74,7 +74,7 @@ class TBindingTraverser : public TLiveTraverser { public: TBindingTraverser(const TIntermediate& i, TBindingMap& bindingMap, TUsedBindings& usedBindings, bool traverseDeadCode = false) : - TLiveTraverser(i, traverseDeadCode), + TLiveTraverser(i, traverseDeadCode, true, true, false), bindingMap(bindingMap), usedBindings(usedBindings) { } diff --git a/gtests/Spv.FromFile.cpp b/gtests/Spv.FromFile.cpp index 5ea46c44..9dd21676 100644 --- a/gtests/Spv.FromFile.cpp +++ b/gtests/Spv.FromFile.cpp @@ -48,6 +48,7 @@ struct IoMapData { int baseTextureBinding; int baseUboBinding; bool autoMapBindings; + bool flattenUniforms; }; std::string FileNameAsCustomTestSuffixIoMap( @@ -119,7 +120,8 @@ TEST_P(HlslSemantics, FromFile) GetParam().baseSamplerBinding, GetParam().baseTextureBinding, GetParam().baseUboBinding, - GetParam().autoMapBindings); + GetParam().autoMapBindings, + GetParam().flattenUniforms); } // clang-format off @@ -251,8 +253,9 @@ INSTANTIATE_TEST_CASE_P( INSTANTIATE_TEST_CASE_P( Hlsl, HlslSemantics, ::testing::ValuesIn(std::vector{ - { "spv.register.autoassign.frag", "main_ep", 5, 10, 15, true }, - { "spv.register.noautoassign.frag", "main_ep", 5, 10, 15, false }, + { "spv.register.autoassign.frag", "main_ep", 5, 10, 15, true, false }, + { "spv.register.noautoassign.frag", "main_ep", 5, 10, 15, false, false }, + { "spv.register.autoassign-2.frag", "main", 5, 10, 15, true, true }, }), FileNameAsCustomTestSuffixIoMap ); diff --git a/gtests/TestFixture.h b/gtests/TestFixture.h index 4b364c4b..0a7bf76e 100644 --- a/gtests/TestFixture.h +++ b/gtests/TestFixture.h @@ -248,7 +248,8 @@ public: int baseSamplerBinding, int baseTextureBinding, int baseUboBinding, - bool autoMapBindings) + bool autoMapBindings, + bool flattenUniformArrays) { const EShLanguage kind = GetShaderStage(GetSuffix(shaderName)); @@ -257,6 +258,7 @@ public: shader.setShiftTextureBinding(baseTextureBinding); shader.setShiftUboBinding(baseUboBinding); shader.setAutoMapBindings(autoMapBindings); + shader.setFlattenUniformArrays(flattenUniformArrays); bool success = compile(&shader, code, entryPointName, controls); @@ -433,7 +435,8 @@ public: int baseSamplerBinding, int baseTextureBinding, int baseUboBinding, - bool autoMapBindings) + bool autoMapBindings, + bool flattenUniformArrays) { const std::string inputFname = testDir + "/" + testName; const std::string expectedOutputFname = @@ -446,7 +449,8 @@ public: const EShMessages controls = DeriveOptions(source, semantics, target); GlslangResult result = compileLinkIoMap(testName, input, entryPointName, controls, baseSamplerBinding, baseTextureBinding, baseUboBinding, - autoMapBindings); + autoMapBindings, + flattenUniformArrays); // Generate the hybrid output in the way of glslangValidator. std::ostringstream stream; diff --git a/hlsl/hlslParseHelper.cpp b/hlsl/hlslParseHelper.cpp index d5db6b23..470b03f5 100755 --- a/hlsl/hlslParseHelper.cpp +++ b/hlsl/hlslParseHelper.cpp @@ -383,6 +383,7 @@ TIntermTyped* HlslParseContext::handleBracketDereference(const TSourceLoc& loc, { TIntermTyped* result = nullptr; + bool flattened = false; int indexValue = 0; if (index->getQualifier().storage == EvqConst) { indexValue = index->getAsConstantUnion()->getConstArray()[0].getIConst(); @@ -408,6 +409,7 @@ TIntermTyped* HlslParseContext::handleBracketDereference(const TSourceLoc& loc, error(loc, "Invalid variable index to flattened uniform array", base->getAsSymbolNode()->getName().c_str(), ""); result = flattenAccess(base, indexValue); + flattened = (result != base); } else { if (index->getQualifier().storage == EvqConst) { if (base->getType().isImplicitlySizedArray()) @@ -423,13 +425,18 @@ TIntermTyped* HlslParseContext::handleBracketDereference(const TSourceLoc& loc, // Insert dummy error-recovery result result = intermediate.addConstantUnion(0.0, EbtFloat, loc); } else { - // Insert valid dereferenced result - TType newType(base->getType(), 0); // dereferenced type - if (base->getType().getQualifier().storage == EvqConst && index->getQualifier().storage == EvqConst) - newType.getQualifier().storage = EvqConst; - else - newType.getQualifier().storage = EvqTemporary; - result->setType(newType); + // If the array reference was flattened, it has the correct type. E.g, if it was + // a uniform array, it was flattened INTO a set of scalar uniforms, not scalar temps. + // In that case, we preserve the qualifiers. + if (!flattened) { + // Insert valid dereferenced result + TType newType(base->getType(), 0); // dereferenced type + if (base->getType().getQualifier().storage == EvqConst && index->getQualifier().storage == EvqConst) + newType.getQualifier().storage = EvqConst; + else + newType.getQualifier().storage = EvqTemporary; + result->setType(newType); + } } return result;