From: John Kessenich Date: Thu, 18 May 2017 00:28:19 +0000 (-0600) Subject: SPV: Give error on not assigning locations to I/O. X-Git-Tag: upstream/11.4.0~1158^2 X-Git-Url: http://review.tizen.org/git/?a=commitdiff_plain;h=71facdf435fac7f688c7237b053be1bef8cd16d0;p=platform%2Fupstream%2Fglslang.git SPV: Give error on not assigning locations to I/O. Also, provides an option to auto-assign locations. Existing tests use this option, to avoid the error message, however, it is not fully implemented yet. --- diff --git a/StandAlone/StandAlone.cpp b/StandAlone/StandAlone.cpp index d713e76..8a6b12d 100644 --- a/StandAlone/StandAlone.cpp +++ b/StandAlone/StandAlone.cpp @@ -90,6 +90,7 @@ enum TOptions { EOptionKeepUncalled = (1 << 22), EOptionHlslOffsets = (1 << 23), EOptionHlslIoMapping = (1 << 24), + EOptionAutoMapLocations = (1 << 25), }; // @@ -386,6 +387,9 @@ void ProcessArguments(std::vector>& workItem lowerword == "hlsl-iomapper" || lowerword == "hlsl-iomapping") { Options |= EOptionHlslIoMapping; + } else if (lowerword == "auto-map-locations" || // synonyms + lowerword == "aml") { + Options |= EOptionAutoMapLocations; } else { usage(); } @@ -648,6 +652,9 @@ void CompileAndLinkShaderUnits(std::vector compUnits) if (Options & EOptionAutoMapBindings) shader->setAutoMapBindings(true); + if (Options & EOptionAutoMapLocations) + shader->setAutoMapLocations(true); + shaders.push_back(shader); const int defaultVersion = Options & EOptionDefaultDesktop? 110: 100; @@ -1063,6 +1070,11 @@ void usage() " explicit bindings.\n" " --amb synonym for --auto-map-bindings\n" "\n" + " --auto-map-locations automatically locate input/output lacking 'location'\n" + " (fragile, not cross stage: recommend explicit\n" + " 'location' use in shader)\n" + " --aml synonym for --auto-map-locations\n" + "\n" " --flatten-uniform-arrays flatten uniform texture & sampler arrays to scalars\n" " --fua synonym for --flatten-uniform-arrays\n" "\n" diff --git a/Test/baseResults/spv.glsl.register.autoassign.frag.out b/Test/baseResults/spv.glsl.register.autoassign.frag.out index 11818f6..8216e05 100644 --- a/Test/baseResults/spv.glsl.register.autoassign.frag.out +++ b/Test/baseResults/spv.glsl.register.autoassign.frag.out @@ -78,6 +78,7 @@ Warning, version 450 is not yet complete; most version-specific features are pre Decorate 126(g_tTex_unused2) DescriptorSet 0 Decorate 126(g_tTex_unused2) Binding 12 Decorate 128(g_sSamp_unused2) DescriptorSet 0 + Decorate 137(FragColor) Location 0 Decorate 141(g_tTex_unused3) DescriptorSet 0 2: TypeVoid 3: TypeFunction 2 diff --git a/Test/baseResults/spv.glsl.register.noautoassign.frag.out b/Test/baseResults/spv.glsl.register.noautoassign.frag.out index 327ac04..8595a89 100644 --- a/Test/baseResults/spv.glsl.register.noautoassign.frag.out +++ b/Test/baseResults/spv.glsl.register.noautoassign.frag.out @@ -72,6 +72,7 @@ Warning, version 450 is not yet complete; most version-specific features are pre Decorate 126(g_tTex_unused2) DescriptorSet 0 Decorate 126(g_tTex_unused2) Binding 12 Decorate 128(g_sSamp_unused2) DescriptorSet 0 + Decorate 137(FragColor) Location 0 Decorate 141(g_tTex_unused3) DescriptorSet 0 2: TypeVoid 3: TypeFunction 2 diff --git a/Test/baseResults/spv.noLocation.vert.out b/Test/baseResults/spv.noLocation.vert.out new file mode 100644 index 0000000..43e2534 --- /dev/null +++ b/Test/baseResults/spv.noLocation.vert.out @@ -0,0 +1,8 @@ +spv.noLocation.vert +Warning, version 450 is not yet complete; most version-specific features are present, but some are missing. +ERROR: spv.noLocation.vert:4: 'location' : SPIR-V requires location for user input/output +ERROR: spv.noLocation.vert:8: 'location' : SPIR-V requires location for user input/output +ERROR: 2 compilation errors. No code generated. + + +SPIR-V is not generated for failed compile or link diff --git a/Test/runtests b/Test/runtests index 4edb8f0..a3c89e0 100755 --- a/Test/runtests +++ b/Test/runtests @@ -86,13 +86,20 @@ $EXE -i --hlsl-offsets -D -e main -H hlsl.hlslOffset.vert > $TARGETDIR/hlsl.hls diff -b $BASEDIR/hlsl.hlslOffset.vert.out $TARGETDIR/hlsl.hlslOffset.vert.out || HASERROR=1 # -# Tesing --resource-set-binding +# Testing --resource-set-binding # echo Configuring HLSL descriptor set and binding number manually $EXE -V -D -e main -H hlsl.multiDescriptorSet.frag --rsb frag t0 0 0 t1 1 0 s0 0 1 s1 1 1 b0 2 0 b1 2 1 b2 2 2 > $TARGETDIR/hlsl.multiDescriptorSet.frag.out diff -b $BASEDIR/hlsl.multiDescriptorSet.frag.out $TARGETDIR/hlsl.multiDescriptorSet.frag.out # +# Testing location error +# +echo Testing SPV no location +$EXE -V -C spv.noLocation.vert > $TARGETDIR/spv.noLocation.vert.out +diff -b $BASEDIR/spv.noLocation.vert.out $TARGETDIR/spv.noLocation.vert.out + +# # Final checking # if [ $HASERROR -eq 0 ] diff --git a/Test/spv.noLocation.vert b/Test/spv.noLocation.vert new file mode 100644 index 0000000..e3c02ee --- /dev/null +++ b/Test/spv.noLocation.vert @@ -0,0 +1,14 @@ +#version 450 + +layout(location = 1) in vec4 in1; +in vec4 in2; +layout(location = 3) in vec4 in3; + +layout(location = 1) out vec4 out1; +out vec4 out2; +layout(location = 3) out vec4 out3; + + +void main() +{ +} diff --git a/glslang/MachineIndependent/ParseHelper.cpp b/glslang/MachineIndependent/ParseHelper.cpp index b7240ea..b3dbeda 100644 --- a/glslang/MachineIndependent/ParseHelper.cpp +++ b/glslang/MachineIndependent/ParseHelper.cpp @@ -4332,6 +4332,18 @@ void TParseContext::layoutObjectCheck(const TSourceLoc& loc, const TSymbol& symb default: break; } + } else if (spvVersion.spv > 0) { + switch (qualifier.storage) { + case EvqVaryingIn: + case EvqVaryingOut: + if (! parsingBuiltins && qualifier.builtIn == EbvNone) { + if (!intermediate.getAutoMapLocations()) + error(loc, "SPIR-V requires location for user input/output", "location", ""); + } + break; + default: + break; + } } // Check packing and matrix @@ -5022,6 +5034,8 @@ TIntermNode* TParseContext::declareVariable(const TSourceLoc& loc, TString& iden // look for errors in layout qualifier use layoutObjectCheck(loc, *symbol); + + // fix up fixOffset(loc, *symbol); return initNode; @@ -5728,6 +5742,7 @@ void TParseContext::declareBlock(const TSourceLoc& loc, TTypeList& typeList, con // Check for general layout qualifier errors layoutObjectCheck(loc, variable); + // fix up if (isIoResizeArray(blockType)) { ioArraySymbolResizeList.push_back(&variable); checkIoArraysConsistency(loc, true); diff --git a/glslang/MachineIndependent/ShaderLang.cpp b/glslang/MachineIndependent/ShaderLang.cpp index 5563da8..1f7c005 100644 --- a/glslang/MachineIndependent/ShaderLang.cpp +++ b/glslang/MachineIndependent/ShaderLang.cpp @@ -1571,6 +1571,8 @@ void TShader::setShiftUavBinding(unsigned int base) { intermediate->setShift void TShader::setShiftSsboBinding(unsigned int base) { intermediate->setShiftSsboBinding(base); } // Enables binding automapping using TIoMapper void TShader::setAutoMapBindings(bool map) { intermediate->setAutoMapBindings(map); } +// Fragile: currently within one stage: simple auto-assignment of location +void TShader::setAutoMapLocations(bool map) { intermediate->setAutoMapLocations(map); } // See comment above TDefaultHlslIoMapper in iomapper.cpp: void TShader::setHlslIoMapping(bool hlslIoMap) { intermediate->setHlslIoMapping(hlslIoMap); } void TShader::setFlattenUniformArrays(bool flatten) { intermediate->setFlattenUniformArrays(flatten); } diff --git a/glslang/MachineIndependent/iomapper.cpp b/glslang/MachineIndependent/iomapper.cpp index 0e7dea1..26772dd 100644 --- a/glslang/MachineIndependent/iomapper.cpp +++ b/glslang/MachineIndependent/iomapper.cpp @@ -214,6 +214,8 @@ struct TNotifyUniformAdaptor { resolver.notifyBinding(stage, ent.symbol->getName().c_str(), ent.symbol->getType(), ent.live); } +private: + TNotifyUniformAdaptor& operator=(TNotifyUniformAdaptor&); }; struct TNotifyInOutAdaptor @@ -229,6 +231,8 @@ struct TNotifyInOutAdaptor { resolver.notifyInOut(stage, ent.symbol->getName().c_str(), ent.symbol->getType(), ent.live); } +private: + TNotifyInOutAdaptor& operator=(TNotifyInOutAdaptor&); }; struct TResolverUniformAdaptor @@ -350,7 +354,8 @@ struct TDefaultIoResolverBase : public glslang::TIoMapResolver int baseSsboBinding; int baseUavBinding; std::vector baseResourceSetBinding; - bool doAutoMapping; + bool doAutoBindingMapping; + bool doAutoLocationMapping; typedef std::vector TSlotSet; typedef std::unordered_map TSlotSetMap; TSlotSetMap slots; @@ -401,9 +406,19 @@ struct TDefaultIoResolverBase : public glslang::TIoMapResolver { return true; } - int resolveInOutLocation(EShLanguage /*stage*/, const char* /*name*/, const TType& /*type*/, bool /*is_live*/) override + int resolveInOutLocation(EShLanguage /*stage*/, const char* /*name*/, const TType& type, bool /*is_live*/) override { - return -1; + if (!doAutoLocationMapping || type.getQualifier().hasLocation()) + return -1; + + // Placeholder. + // TODO: It would be nice to flesh this out using + // intermediate->computeTypeLocationSize(type), or functions that call it like + // intermediate->addUsedLocation() + // These in turn would want the intermediate, which is not available here, but + // is available in many places, and a lot of copying from it could be saved if + // it were just available. + return 0; } int resolveInOutComponent(EShLanguage /*stage*/, const char* /*name*/, const TType& /*type*/, bool /*is_live*/) override { @@ -493,7 +508,7 @@ struct TDefaultIoResolver : public TDefaultIoResolverBase if (isUboType(type)) return reserveSlot(set, baseUboBinding + type.getQualifier().layoutBinding); - } else if (is_live && doAutoMapping) { + } else if (is_live && doAutoBindingMapping) { // find free slot, the caller did make sure it passes all vars with binding // first and now all are passed that do not have a binding and needs one @@ -607,7 +622,7 @@ struct TDefaultHlslIoResolver : public TDefaultIoResolverBase if (isUboType(type)) return reserveSlot(set, baseUboBinding + type.getQualifier().layoutBinding); - } else if (is_live && doAutoMapping) { + } else if (is_live && doAutoBindingMapping) { // find free slot, the caller did make sure it passes all vars with binding // first and now all are passed that do not have a binding and needs one @@ -659,6 +674,7 @@ bool TIoMapper::addStage(EShLanguage stage, TIntermediate &intermediate, TInfoSi intermediate.getShiftUavBinding() == 0 && intermediate.getResourceSetBinding().empty() && intermediate.getAutoMapBindings() == false && + intermediate.getAutoMapLocations() == false && resolver == nullptr) return true; @@ -689,7 +705,8 @@ bool TIoMapper::addStage(EShLanguage stage, TIntermediate &intermediate, TInfoSi resolverBase->baseSsboBinding = intermediate.getShiftSsboBinding(); resolverBase->baseUavBinding = intermediate.getShiftUavBinding(); resolverBase->baseResourceSetBinding = intermediate.getResourceSetBinding(); - resolverBase->doAutoMapping = intermediate.getAutoMapBindings(); + resolverBase->doAutoBindingMapping = intermediate.getAutoMapBindings(); + resolverBase->doAutoLocationMapping = intermediate.getAutoMapLocations(); resolver = resolverBase; } diff --git a/glslang/MachineIndependent/linkValidate.cpp b/glslang/MachineIndependent/linkValidate.cpp index 4bb2951..02dde9e 100644 --- a/glslang/MachineIndependent/linkValidate.cpp +++ b/glslang/MachineIndependent/linkValidate.cpp @@ -860,7 +860,7 @@ int TIntermediate::checkLocationRange(int set, const TIoRange& range, const TTyp return -1; // no collision } -// Accumulate locations used for inputs, outputs, and uniforms, and check for collisions +// Accumulate bindings and offsets, and check for collisions // as the accumulation is done. // // Returns < 0 if no collision, >= 0 if collision and the value returned is a colliding value. diff --git a/glslang/MachineIndependent/localintermediate.h b/glslang/MachineIndependent/localintermediate.h index c28d02a..444d341 100644 --- a/glslang/MachineIndependent/localintermediate.h +++ b/glslang/MachineIndependent/localintermediate.h @@ -177,6 +177,7 @@ public: shiftSsboBinding(0), shiftUavBinding(0), autoMapBindings(false), + autoMapLocations(false), flattenUniformArrays(false), useUnknownFormat(false), hlslOffsets(false), @@ -218,9 +219,11 @@ public: unsigned int getShiftUavBinding() const { return shiftUavBinding; } void setResourceSetBinding(const std::vector& shift) { resourceSetBinding = shift; } const std::vector& getResourceSetBinding() const { return resourceSetBinding; } - void setAutoMapBindings(bool map) { autoMapBindings = map; } + void setAutoMapBindings(bool map) { autoMapBindings = map; } bool getAutoMapBindings() const { return autoMapBindings; } - void setFlattenUniformArrays(bool flatten) { flattenUniformArrays = flatten; } + void setAutoMapLocations(bool map) { autoMapLocations = map; } + bool getAutoMapLocations() const { return autoMapLocations; } + void setFlattenUniformArrays(bool flatten) { flattenUniformArrays = flatten; } bool getFlattenUniformArrays() const { return flattenUniformArrays; } void setNoStorageFormat(bool b) { useUnknownFormat = b; } bool getNoStorageFormat() const { return useUnknownFormat; } @@ -516,6 +519,7 @@ protected: unsigned int shiftUavBinding; std::vector resourceSetBinding; bool autoMapBindings; + bool autoMapLocations; bool flattenUniformArrays; bool useUnknownFormat; bool hlslOffsets; diff --git a/glslang/Public/ShaderLang.h b/glslang/Public/ShaderLang.h index 42e4a46..3be66d4 100644 --- a/glslang/Public/ShaderLang.h +++ b/glslang/Public/ShaderLang.h @@ -309,6 +309,7 @@ public: void setShiftSsboBinding(unsigned int base); void setResourceSetBinding(const std::vector& base); void setAutoMapBindings(bool map); + void setAutoMapLocations(bool map); void setHlslIoMapping(bool hlslIoMap); void setFlattenUniformArrays(bool flatten); void setNoStorageFormat(bool useUnknownFormat); diff --git a/gtests/Link.FromFile.Vk.cpp b/gtests/Link.FromFile.Vk.cpp index 6ce1fe9..6e1969a 100644 --- a/gtests/Link.FromFile.Vk.cpp +++ b/gtests/Link.FromFile.Vk.cpp @@ -60,6 +60,7 @@ TEST_P(LinkTestVulkan, FromFile) shaders.emplace_back( new glslang::TShader(GetShaderStage(GetSuffix(fileNames[i])))); auto* shader = shaders.back().get(); + shader->setAutoMapLocations(true); compile(shader, contents, "", controls); result.shaderResults.push_back( {fileNames[i], shader->getInfoLog(), shader->getInfoDebugLog()}); diff --git a/gtests/TestFixture.h b/gtests/TestFixture.h index 38ec54c..c00645b 100644 --- a/gtests/TestFixture.h +++ b/gtests/TestFixture.h @@ -202,6 +202,7 @@ public: const EShLanguage kind = GetShaderStage(GetSuffix(shaderName)); glslang::TShader shader(kind); + shader.setAutoMapLocations(true); shader.setFlattenUniformArrays(flattenUniformArrays); bool success = compile(&shader, code, entryPointName, controls); @@ -254,6 +255,7 @@ public: shader.setShiftUboBinding(baseUboBinding); shader.setShiftSsboBinding(baseSsboBinding); shader.setAutoMapBindings(autoMapBindings); + shader.setAutoMapLocations(true); shader.setFlattenUniformArrays(flattenUniformArrays); bool success = compile(&shader, code, entryPointName, controls); @@ -295,6 +297,8 @@ public: const EShLanguage kind = GetShaderStage(GetSuffix(shaderName)); glslang::TShader shader(kind); + shader.setAutoMapLocations(true); + bool success = compile(&shader, code, entryPointName, controls); glslang::TProgram program;