From 73c57bbe50f4d4046dd5b4d24b352b003fdbd7bc Mon Sep 17 00:00:00 2001 From: LoopDawg Date: Thu, 5 Oct 2017 16:25:52 -0600 Subject: [PATCH] HLSL: split textures used for both shadow and non-shadow modes A single texture can statically appear in code mixed with a shadow sampler and a non-shadow sampler. This would be create invalid SPIR-V, unless one of them is provably dead. The previous detection of this happened before DCE, so some shaders would trigger the error even though they wouldn't after DCE. To handle that case, this PR splits the texture into two: one with each mode. It sets "needsLegalization" (if that happens for any texture) to warn that this shader will need post-compilation legalization. If the texture is only used with one of the two modes, behavior is as it was before. --- .../hlsl.samplecmp.dualmode.frag.out | 157 ++++++++++++++++++ Test/hlsl.samplecmp.dualmode.frag | 14 ++ glslang/Include/intermediate.h | 1 + gtests/Hlsl.FromFile.cpp | 1 + hlsl/hlslParseHelper.cpp | 94 +++++++---- hlsl/hlslParseHelper.h | 27 ++- 6 files changed, 260 insertions(+), 34 deletions(-) create mode 100644 Test/baseResults/hlsl.samplecmp.dualmode.frag.out create mode 100644 Test/hlsl.samplecmp.dualmode.frag diff --git a/Test/baseResults/hlsl.samplecmp.dualmode.frag.out b/Test/baseResults/hlsl.samplecmp.dualmode.frag.out new file mode 100644 index 000000000..e8024a04a --- /dev/null +++ b/Test/baseResults/hlsl.samplecmp.dualmode.frag.out @@ -0,0 +1,157 @@ +hlsl.samplecmp.dualmode.frag +WARNING: AST will form illegal SPIR-V; need to transform to legalize +Shader version: 500 +gl_FragCoord origin is upper left +0:? Sequence +0:7 Function Definition: @main( ( temp 4-component vector of float) +0:7 Function Parameters: +0:? Sequence +0:10 texture ( temp float) +0:10 Construct combined texture-sampler ( temp sampler1DShadow) +0:10 'g_tTex' (layout( binding=3) uniform texture1DShadow) +0:10 'g_sSampCmp' (layout( binding=1) uniform sampler) +0:10 Construct vec2 ( temp 2-component vector of float) +0:10 Constant: +0:10 0.100000 +0:10 Constant: +0:10 0.750000 +0:11 texture ( temp 4-component vector of float) +0:11 Construct combined texture-sampler ( temp sampler1D) +0:11 'g_tTex' (layout( binding=3) uniform texture1D) +0:11 'g_sSamp' (layout( binding=0) uniform sampler) +0:11 Constant: +0:11 0.100000 +0:13 Branch: Return with expression +0:13 Constant: +0:13 0.000000 +0:13 0.000000 +0:13 0.000000 +0:13 0.000000 +0:7 Function Definition: main( ( temp void) +0:7 Function Parameters: +0:? Sequence +0:7 move second child to first child ( temp 4-component vector of float) +0:? '@entryPointOutput' (layout( location=0) out 4-component vector of float) +0:7 Function Call: @main( ( temp 4-component vector of float) +0:? Linker Objects +0:? 'g_sSamp' (layout( binding=0) uniform sampler) +0:? 'g_sSampCmp' (layout( binding=1) uniform sampler) +0:? 'g_tTex' (layout( binding=3) uniform texture1DShadow) +0:? '@entryPointOutput' (layout( location=0) out 4-component vector of float) +0:? 'g_tTex' (layout( binding=3) uniform texture1D) + + +Linked fragment stage: + + +Shader version: 500 +gl_FragCoord origin is upper left +0:? Sequence +0:7 Function Definition: @main( ( temp 4-component vector of float) +0:7 Function Parameters: +0:? Sequence +0:10 texture ( temp float) +0:10 Construct combined texture-sampler ( temp sampler1DShadow) +0:10 'g_tTex' (layout( binding=3) uniform texture1DShadow) +0:10 'g_sSampCmp' (layout( binding=1) uniform sampler) +0:10 Construct vec2 ( temp 2-component vector of float) +0:10 Constant: +0:10 0.100000 +0:10 Constant: +0:10 0.750000 +0:11 texture ( temp 4-component vector of float) +0:11 Construct combined texture-sampler ( temp sampler1D) +0:11 'g_tTex' (layout( binding=3) uniform texture1D) +0:11 'g_sSamp' (layout( binding=0) uniform sampler) +0:11 Constant: +0:11 0.100000 +0:13 Branch: Return with expression +0:13 Constant: +0:13 0.000000 +0:13 0.000000 +0:13 0.000000 +0:13 0.000000 +0:7 Function Definition: main( ( temp void) +0:7 Function Parameters: +0:? Sequence +0:7 move second child to first child ( temp 4-component vector of float) +0:? '@entryPointOutput' (layout( location=0) out 4-component vector of float) +0:7 Function Call: @main( ( temp 4-component vector of float) +0:? Linker Objects +0:? 'g_sSamp' (layout( binding=0) uniform sampler) +0:? 'g_sSampCmp' (layout( binding=1) uniform sampler) +0:? 'g_tTex' (layout( binding=3) uniform texture1DShadow) +0:? '@entryPointOutput' (layout( location=0) out 4-component vector of float) +0:? 'g_tTex' (layout( binding=3) uniform texture1D) + +// Module Version 10000 +// Generated by (magic number): 80001 +// Id's are bound by 43 + + Capability Shader + Capability Sampled1D + 1: ExtInstImport "GLSL.std.450" + MemoryModel Logical GLSL450 + EntryPoint Fragment 4 "main" 41 + ExecutionMode 4 OriginUpperLeft + Source HLSL 500 + Name 4 "main" + Name 9 "@main(" + Name 13 "g_tTex" + Name 17 "g_sSampCmp" + Name 29 "g_tTex" + Name 31 "g_sSamp" + Name 41 "@entryPointOutput" + Decorate 13(g_tTex) DescriptorSet 0 + Decorate 13(g_tTex) Binding 3 + Decorate 17(g_sSampCmp) DescriptorSet 0 + Decorate 17(g_sSampCmp) Binding 1 + Decorate 29(g_tTex) DescriptorSet 0 + Decorate 29(g_tTex) Binding 3 + Decorate 31(g_sSamp) DescriptorSet 0 + Decorate 31(g_sSamp) Binding 0 + Decorate 41(@entryPointOutput) Location 0 + 2: TypeVoid + 3: TypeFunction 2 + 6: TypeFloat 32 + 7: TypeVector 6(float) 4 + 8: TypeFunction 7(fvec4) + 11: TypeImage 6(float) 1D depth sampled format:Unknown + 12: TypePointer UniformConstant 11 + 13(g_tTex): 12(ptr) Variable UniformConstant + 15: TypeSampler + 16: TypePointer UniformConstant 15 + 17(g_sSampCmp): 16(ptr) Variable UniformConstant + 19: TypeSampledImage 11 + 21: 6(float) Constant 1036831949 + 22: 6(float) Constant 1061158912 + 23: TypeVector 6(float) 2 + 27: TypeImage 6(float) 1D sampled format:Unknown + 28: TypePointer UniformConstant 27 + 29(g_tTex): 28(ptr) Variable UniformConstant + 31(g_sSamp): 16(ptr) Variable UniformConstant + 33: TypeSampledImage 27 + 36: 6(float) Constant 0 + 37: 7(fvec4) ConstantComposite 36 36 36 36 + 40: TypePointer Output 7(fvec4) +41(@entryPointOutput): 40(ptr) Variable Output + 4(main): 2 Function None 3 + 5: Label + 42: 7(fvec4) FunctionCall 9(@main() + Store 41(@entryPointOutput) 42 + Return + FunctionEnd + 9(@main(): 7(fvec4) Function None 8 + 10: Label + 14: 11 Load 13(g_tTex) + 18: 15 Load 17(g_sSampCmp) + 20: 19 SampledImage 14 18 + 24: 23(fvec2) CompositeConstruct 21 22 + 25: 6(float) CompositeExtract 24 1 + 26: 6(float) ImageSampleDrefImplicitLod 20 24 25 + 30: 27 Load 29(g_tTex) + 32: 15 Load 31(g_sSamp) + 34: 33 SampledImage 30 32 + 35: 7(fvec4) ImageSampleImplicitLod 34 21 + ReturnValue 37 + FunctionEnd diff --git a/Test/hlsl.samplecmp.dualmode.frag b/Test/hlsl.samplecmp.dualmode.frag new file mode 100644 index 000000000..5b600f3b6 --- /dev/null +++ b/Test/hlsl.samplecmp.dualmode.frag @@ -0,0 +1,14 @@ +SamplerState g_sSamp : register(s0); +SamplerComparisonState g_sSampCmp : register(s1); + +uniform Texture1D g_tTex : register(t3); + +float4 main() : SV_Target0 +{ + // This texture is used with both shadow modes. It will need post-compilation + // legalization. + g_tTex.SampleCmp(g_sSampCmp, 0.1, 0.75); + g_tTex.Sample(g_sSamp, 0.1); + + return 0; +} diff --git a/glslang/Include/intermediate.h b/glslang/Include/intermediate.h index 4a2b53047..b6c7e8926 100644 --- a/glslang/Include/intermediate.h +++ b/glslang/Include/intermediate.h @@ -979,6 +979,7 @@ public: constSubtree(nullptr) { name = n; } virtual int getId() const { return id; } + virtual void setId(int newId) { id = newId; } virtual const TString& getName() const { return name; } virtual void traverse(TIntermTraverser*); virtual TIntermSymbol* getAsSymbolNode() { return this; } diff --git a/gtests/Hlsl.FromFile.cpp b/gtests/Hlsl.FromFile.cpp index 97101af8a..50b9bc147 100644 --- a/gtests/Hlsl.FromFile.cpp +++ b/gtests/Hlsl.FromFile.cpp @@ -269,6 +269,7 @@ INSTANTIATE_TEST_CASE_P( {"hlsl.samplebias.offsetarray.dx10.frag", "main"}, {"hlsl.samplecmp.array.dx10.frag", "main"}, {"hlsl.samplecmp.basic.dx10.frag", "main"}, + {"hlsl.samplecmp.dualmode.frag", "main"}, {"hlsl.samplecmp.offset.dx10.frag", "main"}, {"hlsl.samplecmp.offsetarray.dx10.frag", "main"}, {"hlsl.samplecmp.negative.frag", "main"}, diff --git a/hlsl/hlslParseHelper.cpp b/hlsl/hlslParseHelper.cpp index 615776c1d..6d0d3f6b5 100755 --- a/hlsl/hlslParseHelper.cpp +++ b/hlsl/hlslParseHelper.cpp @@ -2936,37 +2936,66 @@ TIntermAggregate* HlslParseContext::handleSamplerTextureCombine(const TSourceLoc TSampler samplerType = argTex->getType().getSampler(); samplerType.combined = true; - samplerType.shadow = argSampler->getType().getSampler().shadow; + // TODO: + // This block exists until the spec no longer requires shadow modes on texture objects. + // It can be deleted after that, along with the shadowTextureVariant member. { - // ** TODO: ** - // This forces the texture's shadow state to be the sampler's - // shadow state. This can't work if a single texture is used with - // both comparison and non-comparison samplers, so an error is - // reported if the shader does that. - // - // If this code is ever removed (possibly due to a relaxation in the - // SPIR-V rules), also remove the textureShadowMode member variable. + const bool shadowMode = argSampler->getType().getSampler().shadow; + TIntermSymbol* texSymbol = argTex->getAsSymbolNode(); if (texSymbol == nullptr) texSymbol = argTex->getAsBinaryNode()->getLeft()->getAsSymbolNode(); - if (texSymbol != nullptr) { - const auto textureShadowModeEntry = textureShadowMode.find(texSymbol->getId()); - - // Check to see if this texture has been given a different shadow mode already. - if (textureShadowModeEntry != textureShadowMode.end() && - textureShadowModeEntry->second != samplerType.shadow) { - error(loc, "all uses of texture must use the same shadow mode", "", ""); - return nullptr; - } - - argTex->getWritableType().getSampler().shadow = samplerType.shadow; - textureShadowMode[texSymbol->getId()] = samplerType.shadow; + if (texSymbol == nullptr) { + error(loc, "unable to find texture symbol", "", ""); + return nullptr; } - } + // This forces the texture's shadow state to be the sampler's + // shadow state. This depends on downstream optimization to + // DCE one variant in [shadow, nonshadow] if both are present, + // or the SPIR-V module would be invalid. + int newId = texSymbol->getId(); + + // Check to see if this texture has been given a shadow mode already. + // If so, look up the one we already have. + const auto textureShadowEntry = textureShadowVariant.find(texSymbol->getId()); + + if (textureShadowEntry != textureShadowVariant.end()) + newId = textureShadowEntry->second->get(shadowMode); + else + textureShadowVariant[texSymbol->getId()] = new tShadowTextureSymbols; + + // Sometimes we have to create another symbol (if this texture has been seen before, + // and we haven't created the form for this shadow mode). + if (newId == -1) { + TType texType; + texType.shallowCopy(argTex->getType()); + texType.getSampler().shadow = shadowMode; // set appropriate shadow mode. + globalQualifierFix(loc, texType.getQualifier()); + + TVariable* newTexture = makeInternalVariable(texSymbol->getName(), texType); + + trackLinkage(*newTexture); + + newId = newTexture->getUniqueId(); + } + + assert(newId != -1); + + if (textureShadowVariant.find(newId) == textureShadowVariant.end()) + textureShadowVariant[newId] = textureShadowVariant[texSymbol->getId()]; + + textureShadowVariant[newId]->set(shadowMode, newId); + + // Remember this shadow mode in the texture and the merged type. + argTex->getWritableType().getSampler().shadow = shadowMode; + samplerType.shadow = shadowMode; + + texSymbol->setId(newId); + } txcombine->setType(TType(samplerType, EvqTemporary)); txcombine->setLoc(loc); @@ -9506,14 +9535,19 @@ void HlslParseContext::fixTextureShadowModes() TSampler& sampler = (*symbol)->getWritableType().getSampler(); if (sampler.isTexture()) { - const auto shadowMode = textureShadowMode.find((*symbol)->getUniqueId()); - if (shadowMode != textureShadowMode.end()) - sampler.shadow = shadowMode->second; + const auto shadowMode = textureShadowVariant.find((*symbol)->getUniqueId()); + if (shadowMode != textureShadowVariant.end()) { + + if (shadowMode->second->overloaded()) + // Texture needs legalization if it's been seen with both shadow and non-shadow modes. + intermediate.setNeedsLegalization(); + + sampler.shadow = shadowMode->second->isShadowId((*symbol)->getUniqueId()); + } } } } - // post-processing void HlslParseContext::finish() { @@ -9523,15 +9557,15 @@ void HlslParseContext::finish() error(mipsOperatorMipArg.back().loc, "unterminated mips operator:", "", ""); } + removeUnusedStructBufferCounters(); + addPatchConstantInvocation(); + fixTextureShadowModes(); + // Communicate out (esp. for command line) that we formed AST that will make // illegal AST SPIR-V and it needs transforms to legalize it. if (intermediate.needsLegalization()) infoSink.info << "WARNING: AST will form illegal SPIR-V; need to transform to legalize"; - removeUnusedStructBufferCounters(); - addPatchConstantInvocation(); - fixTextureShadowModes(); - TParseContextBase::finish(); } diff --git a/hlsl/hlslParseHelper.h b/hlsl/hlslParseHelper.h index 2b2245d9b..e4c15a1af 100755 --- a/hlsl/hlslParseHelper.h +++ b/hlsl/hlslParseHelper.h @@ -457,10 +457,29 @@ protected: TVector mipsOperatorMipArg; - // This can be removed if and when the texture shadow workarounnd in - // HlslParseContext::handleSamplerTextureCombine is removed. It maps - // texture symbol IDs to the shadow modes of samplers they were combined with. - TMap textureShadowMode; + // A texture object may be used with shadow and non-shadow samplers, but both may not be + // alive post-DCE in the same shader. We do not know at compilation time which are alive: that's + // only known post-DCE. If a texture is used both ways, we create two textures, and + // leave the elimiation of one to the optimizer. This maps the shader variant to + // the shadow variant. + // + // This can be removed if and when the texture shadow code in + // HlslParseContext::handleSamplerTextureCombine is removed. + struct tShadowTextureSymbols { + tShadowTextureSymbols() { symId.fill(-1); } + + void set(bool shadow, int id) { symId[int(shadow)] = id; } + int get(bool shadow) const { return symId[int(shadow)]; } + + // True if this texture has been seen with both shadow and non-shadow modes + bool overloaded() const { return symId[0] != -1 && symId[1] != -1; } + bool isShadowId(int id) const { return symId[1] == id; } + + private: + std::array symId; + }; + + TMap textureShadowVariant; }; // This is the prefix we use for built-in methods to avoid namespace collisions with