Merge pull request #896 from KhronosGroup/spv-location

SPV: Give error on not assigning locations to I/O when generating SPIR-V.

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<std::unique_ptr<glslang::TWorkItem>>& 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<ShaderCompUnit> 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"

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

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

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

Test/runtests

@@ -86,12 +86,19 @@ $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
 #

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()
+{
+}

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);

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); }

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<std::string> baseResourceSetBinding;
-    bool doAutoMapping;
+    bool doAutoBindingMapping;
+    bool doAutoLocationMapping;
     typedef std::vector<int> TSlotSet;
     typedef std::unordered_map<int, TSlotSet> 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
     {
+        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;
     }

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.

glslang/MachineIndependent/localintermediate.h

@@ -177,6 +177,7 @@ public:
         shiftSsboBinding(0),
         shiftUavBinding(0),
         autoMapBindings(false),
+        autoMapLocations(false),
         flattenUniformArrays(false),
         useUnknownFormat(false),
         hlslOffsets(false),
@@ -220,6 +221,8 @@ public:
     const std::vector<std::string>& getResourceSetBinding() const { return resourceSetBinding; }
     void setAutoMapBindings(bool map)           { autoMapBindings = map; }
     bool getAutoMapBindings()             const { return autoMapBindings; }
+    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; }
@@ -516,6 +519,7 @@ protected:
     unsigned int shiftUavBinding;
     std::vector<std::string> resourceSetBinding;
     bool autoMapBindings;
+    bool autoMapLocations;
     bool flattenUniformArrays;
     bool useUnknownFormat;
     bool hlslOffsets;

glslang/Public/ShaderLang.h

@@ -309,6 +309,7 @@ public:
     void setShiftSsboBinding(unsigned int base);
     void setResourceSetBinding(const std::vector<std::string>& base);
     void setAutoMapBindings(bool map);
+    void setAutoMapLocations(bool map);
     void setHlslIoMapping(bool hlslIoMap);
     void setFlattenUniformArrays(bool flatten);
     void setNoStorageFormat(bool useUnknownFormat);

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()});

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;