Vulkan: Avoid linear search in shader on varying location assignment

ShabbyX · Commit Bot · commit 6381d7f14805 · 2019-12-12T03:45:04.000Z
Glslang wrapper was trying to identify whether the varying is declared in each of the out/in shaders by looping through the macro symbols that need replacement. This change instead adds stage information to PackedVarying assigned when collecting varyings. Glslang wrapper then simply tests the bitfield for the stages of interest. Bug: angleproject:3571 Change-Id: I29614e3e62d7df88e413c1732ac04e24243f167a Reviewed-on: https://chromium-review.googlesource.com/c/angle/angle/+/1954677 Reviewed-by: Shahbaz Youssefi <syoussefi@chromium.org> Commit-Queue: Shahbaz Youssefi <syoussefi@chromium.org>
diff --git a/src/libANGLE/Program.cpp b/src/libANGLE/Program.cpp
@@ -4339,13 +4339,18 @@ ProgramMergedVaryings Program::getMergedVaryings() const
     {
         if (shader)
         {
+            ShaderType shaderType = shader->getType();
             for (const sh::ShaderVariable &varying : shader->getOutputVaryings())
             {
-                merged[varying.name].frontShader = &varying;
+                ProgramVaryingRef *ref = &merged[varying.name];
+                ref->frontShader       = &varying;
+                ref->frontShaderStage  = shaderType;
             }
             for (const sh::ShaderVariable &varying : shader->getInputVaryings())
             {
-                merged[varying.name].backShader = &varying;
+                ProgramVaryingRef *ref = &merged[varying.name];
+                ref->backShader        = &varying;
+                ref->backShaderStage   = shaderType;
             }
         }
     }
diff --git a/src/libANGLE/Program.h b/src/libANGLE/Program.h
@@ -569,6 +569,8 @@ struct ProgramVaryingRef
 
     const sh::ShaderVariable *frontShader = nullptr;
     const sh::ShaderVariable *backShader  = nullptr;
+    ShaderType frontShaderStage           = ShaderType::InvalidEnum;
+    ShaderType backShaderStage            = ShaderType::InvalidEnum;
 };
 
 using ProgramMergedVaryings = std::map<std::string, ProgramVaryingRef>;
diff --git a/src/libANGLE/VaryingPacking.cpp b/src/libANGLE/VaryingPacking.cpp
@@ -55,6 +55,41 @@ bool ComparePackedVarying(const PackedVarying &x, const PackedVarying &y)
 
 }  // anonymous namespace
 
+// Implementation of PackedVarying
+PackedVarying::PackedVarying(const sh::ShaderVariable &varyingIn,
+                             sh::InterpolationType interpolationIn)
+    : PackedVarying(varyingIn, interpolationIn, "", false)
+{}
+PackedVarying::PackedVarying(const sh::ShaderVariable &varyingIn,
+                             sh::InterpolationType interpolationIn,
+                             const std::string &parentStructNameIn,
+                             GLuint fieldIndexIn)
+    : varying(&varyingIn),
+      interpolation(interpolationIn),
+      parentStructName(parentStructNameIn),
+      arrayIndex(GL_INVALID_INDEX),
+      fieldIndex(fieldIndexIn)
+{}
+
+PackedVarying::~PackedVarying() = default;
+
+PackedVarying::PackedVarying(PackedVarying &&other)
+    : PackedVarying(*other.varying, other.interpolation)
+{
+    *this = std::move(other);
+}
+PackedVarying &PackedVarying::operator=(PackedVarying &&other)
+{
+    std::swap(varying, other.varying);
+    std::swap(shaderStages, other.shaderStages);
+    std::swap(interpolation, other.interpolation);
+    std::swap(parentStructName, other.parentStructName);
+    std::swap(arrayIndex, other.arrayIndex);
+    std::swap(fieldIndex, other.fieldIndex);
+
+    return *this;
+}
+
 // Implementation of VaryingPacking
 VaryingPacking::VaryingPacking(GLuint maxVaryingVectors, PackMode packMode)
     : mRegisterMap(maxVaryingVectors), mPackMode(packMode)
@@ -295,6 +330,10 @@ bool VaryingPacking::collectAndPackUserVaryings(gl::InfoLog &infoLog,
             mInputVaryings.emplace_back(*input);
         }
 
+        ShaderBitSet shaderStages;
+        shaderStages.set(ref.second.frontShaderStage);
+        shaderStages.set(ref.second.backShaderStage);
+
         // Only pack statically used varyings that have a matched input or output, plus special
         // builtins. Note that we pack all statically used user-defined varyings even if they are
         // not active. GLES specs are a bit vague on whether it's allowed to only pack active
@@ -326,19 +365,21 @@ bool VaryingPacking::collectAndPackUserVaryings(gl::InfoLog &infoLog,
                         ASSERT(!field.isStruct() && !field.isArray());
                         mPackedVaryings.emplace_back(field, interpolation, varying->name,
                                                      fieldIndex);
+                        mPackedVaryings.back().shaderStages = shaderStages;
                         uniqueFullNames.insert(mPackedVaryings.back().fullName());
                     }
                 }
                 else
                 {
                     mPackedVaryings.emplace_back(*varying, interpolation);
+                    mPackedVaryings.back().shaderStages = shaderStages;
                     uniqueFullNames.insert(mPackedVaryings.back().fullName());
                 }
                 continue;
             }
         }
 
-        // If the varying is not used in the VS, we know it is inactive.
+        // If the varying is not used in the input, we know it is inactive.
         if (!input)
         {
             mInactiveVaryingNames.push_back(ref.first);
@@ -370,7 +411,7 @@ bool VaryingPacking::collectAndPackUserVaryings(gl::InfoLog &infoLog,
                     ASSERT(!field->isStruct() && !field->isArray());
                     mPackedVaryings.emplace_back(*field, input->interpolation, input->name,
                                                  fieldIndex);
-                    mPackedVaryings.back().vertexOnly = true;
+                    mPackedVaryings.back().shaderStages.set(ShaderType::Vertex);
                     mPackedVaryings.back().arrayIndex = GL_INVALID_INDEX;
                     uniqueFullNames.insert(tfVarying);
                 }
@@ -383,7 +424,7 @@ bool VaryingPacking::collectAndPackUserVaryings(gl::InfoLog &infoLog,
                 if (tfVarying.compare(0, 3, "gl_") != 0)
                 {
                     mPackedVaryings.emplace_back(*input, input->interpolation);
-                    mPackedVaryings.back().vertexOnly = true;
+                    mPackedVaryings.back().shaderStages.set(ShaderType::Vertex);
                     mPackedVaryings.back().arrayIndex = static_cast<GLuint>(subscript);
                     uniqueFullNames.insert(tfVarying);
                 }
diff --git a/src/libANGLE/VaryingPacking.h b/src/libANGLE/VaryingPacking.h
@@ -16,6 +16,7 @@
 
 #include "angle_gl.h"
 #include "common/angleutils.h"
+#include "libANGLE/angletypes.h"
 
 #include <map>
 
@@ -26,22 +27,17 @@ struct ProgramVaryingRef;
 
 using ProgramMergedVaryings = std::map<std::string, ProgramVaryingRef>;
 
-struct PackedVarying
+struct PackedVarying : angle::NonCopyable
 {
-    PackedVarying(const sh::ShaderVariable &varyingIn, sh::InterpolationType interpolationIn)
-        : PackedVarying(varyingIn, interpolationIn, "", false)
-    {}
+    PackedVarying(const sh::ShaderVariable &varyingIn, sh::InterpolationType interpolationIn);
     PackedVarying(const sh::ShaderVariable &varyingIn,
                   sh::InterpolationType interpolationIn,
                   const std::string &parentStructNameIn,
-                  GLuint fieldIndexIn)
-        : varying(&varyingIn),
-          vertexOnly(false),
-          interpolation(interpolationIn),
-          parentStructName(parentStructNameIn),
-          arrayIndex(GL_INVALID_INDEX),
-          fieldIndex(fieldIndexIn)
-    {}
+                  GLuint fieldIndexIn);
+    PackedVarying(PackedVarying &&other);
+    ~PackedVarying();
+
+    PackedVarying &operator=(PackedVarying &&other);
 
     bool isStructField() const { return !parentStructName.empty(); }
 
@@ -63,10 +59,17 @@ struct PackedVarying
         return fullNameStr.str();
     }
 
+    // Transform feedback varyings can be only referenced in the VS.
+    bool vertexOnly() const
+    {
+        ShaderBitSet vertex;
+        vertex.set(ShaderType::Vertex);
+        return shaderStages == vertex;
+    }
+
     const sh::ShaderVariable *varying;
 
-    // Transform feedback varyings can be only referenced in the VS.
-    bool vertexOnly;
+    ShaderBitSet shaderStages;
 
     // Cached so we can store sh::ShaderVariable to point to varying fields.
     sh::InterpolationType interpolation;
diff --git a/src/libANGLE/renderer/d3d/DynamicHLSL.cpp b/src/libANGLE/renderer/d3d/DynamicHLSL.cpp
@@ -803,7 +803,7 @@ void DynamicHLSL::generateShaderLinkHLSL(const gl::Caps &caps,
 
         // Don't reference VS-only transform feedback varyings in the PS. Note that we're relying on
         // that the active flag is set according to usage in the fragment shader.
-        if (packedVarying.vertexOnly || !varying.active)
+        if (packedVarying.vertexOnly() || !varying.active)
             continue;
 
         pixelPrologue << "    ";
diff --git a/src/libANGLE/renderer/glslang_wrapper_utils.cpp b/src/libANGLE/renderer/glslang_wrapper_utils.cpp
@@ -132,7 +132,6 @@ class IntermediateShaderSource final : angle::NonCopyable
     void init(const std::string &source);
     bool empty() const { return mTokens.empty(); }
 
-    bool findTokenName(const std::string &name);
     // Find @@ LAYOUT-name(extra, args) @@ and replace it with:
     //
     //     layout(specifier, extra, args)
@@ -311,18 +310,6 @@ void IntermediateShaderSource::init(const std::string &source)
     }
 }
 
-bool IntermediateShaderSource::findTokenName(const std::string &name)
-{
-    for (Token &block : mTokens)
-    {
-        if (block.text == name)
-        {
-            return true;
-        }
-    }
-    return false;
-}
-
 void IntermediateShaderSource::insertLayoutSpecifier(const std::string &name,
                                                      const std::string &specifier)
 {
@@ -728,10 +715,15 @@ void AssignOutputLocations(const gl::ProgramState &programState,
 
 void AssignVaryingLocations(const gl::ProgramState &programState,
                             const gl::ProgramLinkedResources &resources,
-                            IntermediateShaderSource *outStageSource,
-                            IntermediateShaderSource *inStageSource,
+                            gl::ShaderType outStage,
+                            gl::ShaderType inStage,
+                            gl::ShaderMap<IntermediateShaderSource> *shaderSources,
                             XfbBufferMap *xfbBufferMap)
 {
+
+    IntermediateShaderSource *outStageSource = &(*shaderSources)[outStage];
+    IntermediateShaderSource *inStageSource  = &(*shaderSources)[inStage];
+
     // Assign varying locations.
     for (const gl::PackedVaryingRegister &varyingReg : resources.varyingPacking.getRegisterList())
     {
@@ -768,13 +760,13 @@ void AssignVaryingLocations(const gl::ProgramState &programState,
         const std::string &name =
             varying.isStructField() ? varying.parentStructName : varying.varying->name;
 
-        // Varings are from 3 stage of shader sources
+        // Varyings are from multiple shader stages
         // To match pair of (out - in) qualifier, varying should be in the pair of shader source
-        if (!outStageSource->findTokenName(name) || !inStageSource->findTokenName(name))
+        if (!varying.shaderStages.test(outStage) || !varying.shaderStages.test(inStage))
         {
             // Pair can be unmatching at transform feedback case,
             // But it requires qualifier.
-            if (!varying.vertexOnly)
+            if (!varying.vertexOnly())
                 continue;
         }
 
@@ -1262,28 +1254,28 @@ void GlslangGetShaderSource(const GlslangSourceOptions &options,
     if (!geometrySource->empty())
     {
         AssignOutputLocations(programState, fragmentSource);
-        AssignVaryingLocations(programState, resources, geometrySource, fragmentSource,
-                               &xfbBufferMap);
+        AssignVaryingLocations(programState, resources, gl::ShaderType::Geometry,
+                               gl::ShaderType::Fragment, &intermediateSources, &xfbBufferMap);
         if (!vertexSource->empty())
         {
             AssignAttributeLocations(programState, vertexSource);
-            AssignVaryingLocations(programState, resources, vertexSource, geometrySource,
-                                   &xfbBufferMap);
+            AssignVaryingLocations(programState, resources, gl::ShaderType::Vertex,
+                                   gl::ShaderType::Geometry, &intermediateSources, &xfbBufferMap);
         }
     }
     else if (!vertexSource->empty())
     {
         AssignAttributeLocations(programState, vertexSource);
         AssignOutputLocations(programState, fragmentSource);
-        AssignVaryingLocations(programState, resources, vertexSource, fragmentSource,
-                               &xfbBufferMap);
+        AssignVaryingLocations(programState, resources, gl::ShaderType::Vertex,
+                               gl::ShaderType::Fragment, &intermediateSources, &xfbBufferMap);
     }
     else if (!fragmentSource->empty())
     {
         AssignAttributeLocations(programState, fragmentSource);
         AssignOutputLocations(programState, fragmentSource);
-        AssignVaryingLocations(programState, resources, vertexSource, fragmentSource,
-                               &xfbBufferMap);
+        AssignVaryingLocations(programState, resources, gl::ShaderType::Vertex,
+                               gl::ShaderType::Fragment, &intermediateSources, &xfbBufferMap);
     }
     AssignUniformBindings(options, &intermediateSources);
     AssignTextureBindings(options, useOldRewriteStructSamplers, programState, &intermediateSources);

Original file line number	Diff line number	Diff line change
`@@ -4339,13 +4339,18 @@ ProgramMergedVaryings Program::getMergedVaryings() const`
`4339`	`4339`	`{`
`4340`	`4340`	`if (shader)`
`4341`	`4341`	`{`
	`4342`	`+ ShaderType shaderType = shader->getType();`
`4342`	`4343`	`for (const sh::ShaderVariable &varying : shader->getOutputVaryings())`
`4343`	`4344`	`{`
`4344`		`- merged[varying.name].frontShader = &varying;`
	`4345`	`+ ProgramVaryingRef *ref = &merged[varying.name];`
	`4346`	`+ ref->frontShader = &varying;`
	`4347`	`+ ref->frontShaderStage = shaderType;`
`4345`	`4348`	`}`
`4346`	`4349`	`for (const sh::ShaderVariable &varying : shader->getInputVaryings())`
`4347`	`4350`	`{`
`4348`		`- merged[varying.name].backShader = &varying;`
	`4351`	`+ ProgramVaryingRef *ref = &merged[varying.name];`
	`4352`	`+ ref->backShader = &varying;`
	`4353`	`+ ref->backShaderStage = shaderType;`
`4349`	`4354`	`}`
`4350`	`4355`	`}`
`4351`	`4356`	`}`