Add GLSL support for runtime-sized arrays in SSBOs

src/compiler/translator/IntermNode.cpp

@@ -1390,7 +1390,8 @@ TIntermTyped *TIntermUnary::fold(TDiagnostics *diagnostics)
 
     if (mOp == EOpArrayLength)
     {
-        if (mOperand->hasSideEffects())
+        // The size of runtime-sized arrays may only be determined at runtime.
+        if (mOperand->hasSideEffects() || mOperand->getType().isUnsizedArray())
         {
             return this;
         }

src/compiler/translator/OutputGLSLBase.cpp

@@ -526,24 +526,42 @@ bool TOutputGLSLBase::visitBinary(Visit visit, TIntermBinary *node)
                 }
                 else if (visit == PostVisit)
                 {
-                    int maxSize;
                     TIntermTyped *left = node->getLeft();
                     TType leftType     = left->getType();
 
-                    if (left->isArray())
+                    if (mClampingStrategy == SH_CLAMP_WITH_CLAMP_INTRINSIC)
+                        out << "), 0.0, float(";
+                    else
+                        out << ", 0, ";
+
+                    if (leftType.isUnsizedArray())
                     {
-                        // The shader will fail validation if the array length is not > 0.
-                        maxSize = static_cast<int>(leftType.getOutermostArraySize()) - 1;
+                        // For runtime-sized arrays in ESSL 3.10 we need to call the length method
+                        // to get the length to clamp against. See ESSL 3.10 section 4.1.9. Note
+                        // that a runtime-sized array expression is guaranteed not to have side
+                        // effects, so it's fine to add the expression to the output twice.
+                        ASSERT(mShaderVersion >= 310);
+                        ASSERT(!left->hasSideEffects());
+                        left->traverse(this);
+                        out << ".length() - 1";
                     }
                     else
                     {
-                        maxSize = leftType.getNominalSize() - 1;
+                        int maxSize;
+                        if (leftType.isArray())
+                        {
+                            maxSize = static_cast<int>(leftType.getOutermostArraySize()) - 1;
+                        }
+                        else
+                        {
+                            maxSize = leftType.getNominalSize() - 1;
+                        }
+                        out << maxSize;
                     }
-
                     if (mClampingStrategy == SH_CLAMP_WITH_CLAMP_INTRINSIC)
-                        out << "), 0.0, float(" << maxSize << ")))]";
+                        out << ")))]";
                     else
-                        out << ", 0, " << maxSize << ")]";
+                        out << ")]";
                 }
             }
             else

src/compiler/translator/ParseContext.cpp

@@ -3785,6 +3785,15 @@ TIntermDeclaration *TParseContext::addInterfaceBlock(
 
         fieldType->setLayoutQualifier(fieldLayoutQualifier);
 
+        if (mShaderVersion < 310 || memberIndex != fieldList->size() - 1u ||
+            typeQualifier.qualifier != EvqBuffer)
+        {
+            // ESSL 3.10 spec section 4.1.9 allows for runtime-sized arrays.
+            checkIsNotUnsizedArray(field->line(),
+                                   "array members of interface blocks must specify a size",
+                                   field->name().c_str(), field->type());
+        }
+
         if (typeQualifier.qualifier == EvqBuffer)
         {
             // set memory qualifiers
@@ -4007,85 +4016,88 @@ TIntermTyped *TParseContext::addIndexExpression(TIntermTyped *baseExpression,
 
         int safeIndex = -1;
 
-        if (baseExpression->isArray())
+        if (index < 0)
+        {
+            outOfRangeError(outOfRangeIndexIsError, location, "index expression is negative", "[]");
+            safeIndex = 0;
+        }
+
+        if (!baseExpression->getType().isUnsizedArray())
         {
-            if (baseExpression->getQualifier() == EvqFragData && index > 0)
+            if (baseExpression->isArray())
             {
-                if (!isExtensionEnabled(TExtension::EXT_draw_buffers))
+                if (baseExpression->getQualifier() == EvqFragData && index > 0)
+                {
+                    if (!isExtensionEnabled(TExtension::EXT_draw_buffers))
+                    {
+                        outOfRangeError(outOfRangeIndexIsError, location,
+                                        "array index for gl_FragData must be zero when "
+                                        "GL_EXT_draw_buffers is disabled",
+                                        "[]");
+                        safeIndex = 0;
+                    }
+                }
+                // Only do generic out-of-range check if similar error hasn't already been reported.
+                if (safeIndex < 0)
                 {
-                    outOfRangeError(outOfRangeIndexIsError, location,
-                                    "array index for gl_FragData must be zero when "
-                                    "GL_EXT_draw_buffers is disabled",
-                                    "[");
-                    safeIndex = 0;
+                    safeIndex = checkIndexLessThan(outOfRangeIndexIsError, location, index,
+                                                   baseExpression->getOutermostArraySize(),
+                                                   "array index out of range");
                 }
             }
-            // Only do generic out-of-range check if similar error hasn't already been reported.
-            if (safeIndex < 0)
+            else if (baseExpression->isMatrix())
             {
-                safeIndex = checkIndexOutOfRange(outOfRangeIndexIsError, location, index,
-                                                 baseExpression->getOutermostArraySize(),
-                                                 "array index out of range");
+                safeIndex = checkIndexLessThan(outOfRangeIndexIsError, location, index,
+                                               baseExpression->getType().getCols(),
+                                               "matrix field selection out of range");
+            }
+            else if (baseExpression->isVector())
+            {
+                safeIndex = checkIndexLessThan(outOfRangeIndexIsError, location, index,
+                                               baseExpression->getType().getNominalSize(),
+                                               "vector field selection out of range");
             }
-        }
-        else if (baseExpression->isMatrix())
-        {
-            safeIndex = checkIndexOutOfRange(outOfRangeIndexIsError, location, index,
-                                             baseExpression->getType().getCols(),
-                                             "matrix field selection out of range");
-        }
-        else if (baseExpression->isVector())
-        {
-            safeIndex = checkIndexOutOfRange(outOfRangeIndexIsError, location, index,
-                                             baseExpression->getType().getNominalSize(),
-                                             "vector field selection out of range");
-        }
 
-        ASSERT(safeIndex >= 0);
-        // Data of constant unions can't be changed, because it may be shared with other
-        // constant unions or even builtins, like gl_MaxDrawBuffers. Instead use a new
-        // sanitized object.
-        if (safeIndex != index || indexConstantUnion->getBasicType() != EbtInt)
-        {
-            TConstantUnion *safeConstantUnion = new TConstantUnion();
-            safeConstantUnion->setIConst(safeIndex);
-            indexConstantUnion->replaceConstantUnion(safeConstantUnion);
-            indexConstantUnion->getTypePointer()->setBasicType(EbtInt);
-        }
+            ASSERT(safeIndex >= 0);
+            // Data of constant unions can't be changed, because it may be shared with other
+            // constant unions or even builtins, like gl_MaxDrawBuffers. Instead use a new
+            // sanitized object.
+            if (safeIndex != index || indexConstantUnion->getBasicType() != EbtInt)
+            {
+                TConstantUnion *safeConstantUnion = new TConstantUnion();
+                safeConstantUnion->setIConst(safeIndex);
+                indexConstantUnion->replaceConstantUnion(safeConstantUnion);
+                indexConstantUnion->getTypePointer()->setBasicType(EbtInt);
+            }
 
-        TIntermBinary *node = new TIntermBinary(EOpIndexDirect, baseExpression, indexExpression);
-        node->setLine(location);
-        return node->fold(mDiagnostics);
-    }
-    else
-    {
-        TIntermBinary *node = new TIntermBinary(EOpIndexIndirect, baseExpression, indexExpression);
-        node->setLine(location);
-        // Indirect indexing can never be constant folded.
-        return node;
+            TIntermBinary *node =
+                new TIntermBinary(EOpIndexDirect, baseExpression, indexExpression);
+            node->setLine(location);
+            return node->fold(mDiagnostics);
+        }
     }
+
+    TIntermBinary *node = new TIntermBinary(EOpIndexIndirect, baseExpression, indexExpression);
+    node->setLine(location);
+    // Indirect indexing can never be constant folded.
+    return node;
 }
 
-int TParseContext::checkIndexOutOfRange(bool outOfRangeIndexIsError,
-                                        const TSourceLoc &location,
-                                        int index,
-                                        int arraySize,
-                                        const char *reason)
+int TParseContext::checkIndexLessThan(bool outOfRangeIndexIsError,
+                                      const TSourceLoc &location,
+                                      int index,
+                                      int arraySize,
+                                      const char *reason)
 {
-    if (index >= arraySize || index < 0)
+    // Should not reach here with an unsized / runtime-sized array.
+    ASSERT(arraySize > 0);
+    if (index >= arraySize)
     {
         std::stringstream reasonStream;
         reasonStream << reason << " '" << index << "'";
         std::string token = reasonStream.str();
         outOfRangeError(outOfRangeIndexIsError, location, reason, "[]");
-        if (index < 0)
-        {
-            return 0;
-        }
-        else
-        {
-            return arraySize - 1;
-        }
+        return arraySize - 1;
     }
     return index;
 }
@@ -4759,9 +4771,6 @@ TFieldList *TParseContext::addStructDeclaratorList(const TPublicType &typeSpecif
         {
             type->makeArray(arraySize);
         }
-        checkIsNotUnsizedArray(typeSpecifier.getLine(),
-                               "array members of structs must specify a size",
-                               declarator->name().c_str(), type);
 
         checkIsBelowStructNestingLimit(typeSpecifier.getLine(), *declarator);
     }
@@ -4792,7 +4801,7 @@ TTypeSpecifierNonArray TParseContext::addStructure(const TSourceLoc &structLine,
     // ensure we do not specify any storage qualifiers on the struct members
     for (unsigned int typeListIndex = 0; typeListIndex < fieldList->size(); typeListIndex++)
     {
-        const TField &field        = *(*fieldList)[typeListIndex];
+        TField &field              = *(*fieldList)[typeListIndex];
         const TQualifier qualifier = field.type()->getQualifier();
         switch (qualifier)
         {
@@ -4814,6 +4823,9 @@ TTypeSpecifierNonArray TParseContext::addStructure(const TSourceLoc &structLine,
             error(field.line(), "disallowed type in struct", field.type()->getBasicString());
         }
 
+        checkIsNotUnsizedArray(field.line(), "array members of structs must specify a size",
+                               field.name().c_str(), field.type());
+
         checkMemoryQualifierIsNotSpecified(field.type()->getMemoryQualifier(), field.line());
 
         checkBindingIsNotSpecified(field.line(), field.type()->getLayoutQualifier().binding);

src/compiler/translator/ParseContext.h

@@ -462,11 +462,11 @@ class TParseContext : angle::NonCopyable
     constexpr static size_t kAtomicCounterArrayStride = 4;
 
     // Returns a clamped index. If it prints out an error message, the token is "[]".
-    int checkIndexOutOfRange(bool outOfRangeIndexIsError,
-                             const TSourceLoc &location,
-                             int index,
-                             int arraySize,
-                             const char *reason);
+    int checkIndexLessThan(bool outOfRangeIndexIsError,
+                           const TSourceLoc &location,
+                           int index,
+                           int arraySize,
+                           const char *reason);
 
     bool declareVariable(const TSourceLoc &line,
                          const TString &identifier,

src/compiler/translator/RemoveArrayLengthMethod.cpp

@@ -16,6 +16,8 @@
 //
 //   Must be run after SplitSequenceOperator, SimplifyLoopConditions and SeparateDeclarations steps
 //   have been done to expressions containing calls of the array length method.
+//
+//   Does nothing to length method calls done on runtime-sized arrays.
 
 #include "compiler/translator/RemoveArrayLengthMethod.h"
 
@@ -45,7 +47,8 @@ class RemoveArrayLengthTraverser : public TIntermTraverser
 
 bool RemoveArrayLengthTraverser::visitUnary(Visit visit, TIntermUnary *node)
 {
-    if (node->getOp() == EOpArrayLength)
+    // The only case where we leave array length() in place is for runtime-sized arrays.
+    if (node->getOp() == EOpArrayLength && !node->getOperand()->getType().isUnsizedArray())
     {
         mFoundArrayLength = true;
         if (!node->getOperand()->hasSideEffects())

src/compiler/translator/RemoveArrayLengthMethod.h

@@ -16,6 +16,8 @@
 //
 //   Must be run after SplitSequenceOperator, SimplifyLoopConditions and SeparateDeclarations steps
 //   have been done to expressions containing calls of the array length method.
+//
+//   Does nothing to length method calls done on runtime-sized arrays.
 
 namespace sh
 {

src/compiler/translator/ShaderLang.cpp

@@ -272,6 +272,7 @@ void InitBuiltInResources(ShBuiltInResources *resources)
     resources->MaxAtomicCounterBufferSize      = 32;
 
     resources->MaxUniformBufferBindings = 32;
+    resources->MaxShaderStorageBufferBindings = 4;
 
     resources->MaxGeometryUniformComponents     = 1024;
     resources->MaxGeometryUniformBlocks         = 12;

src/tests/compiler_tests/BufferVariables_test.cpp

@@ -519,3 +519,66 @@ TEST_F(BufferVariablesTest, UniformBlockWithStd430)
         FAIL() << "Shader compilation succeeded, expecting failure:\n" << mInfoLog;
     }
 }
+
+// Test that indexing a runtime-sized array with a positive index compiles.
+TEST_F(BufferVariablesTest, IndexRuntimeSizedArray)
+{
+    const std::string &source =
+        R"(#version 310 es
+
+        layout(std430) buffer buf
+        {
+            int arr[];
+        };
+
+        void main()
+        {
+            arr[100];
+        })";
+    if (!compile(source))
+    {
+        FAIL() << "Shader compilation failed, expecting success:\n" << mInfoLog;
+    }
+}
+
+// Test that indexing a runtime-sized array with a negative constant index does not compile.
+TEST_F(BufferVariablesTest, IndexRuntimeSizedArrayWithNegativeIndex)
+{
+    const std::string &source =
+        R"(#version 310 es
+
+        layout(std430) buffer buf
+        {
+            int arr[];
+        };
+
+        void main()
+        {
+            arr[-1];
+        })";
+    if (compile(source))
+    {
+        FAIL() << "Shader compilation succeeded, expecting failure:\n" << mInfoLog;
+    }
+}
+
+// Test that only the last member of a buffer can be runtime-sized.
+TEST_F(BufferVariablesTest, RuntimeSizedVariableInNotLastInBuffer)
+{
+    const std::string &source =
+        R"(#version 310 es
+
+        layout(std430) buffer buf
+        {
+            int arr[];
+            int i;
+        };
+
+        void main()
+        {
+        })";
+    if (compile(source))
+    {
+        FAIL() << "Shader compilation succeeded, expecting failure:\n" << mInfoLog;
+    }
+}