Reland "Restrict where 'binding' and 'set' can appear"

This is a reland of 9372ef0228

Original change's description:
> Restrict where 'binding' and 'set' can appear
>
> In SPIRV, these are an error when applied to struct members. Some of our
> tests were triggering that because we had free-floating uniforms
> decorated this way (and we coalesce those into members of an interface
> block).
>
> Now, we only allow those layout qualifiers on variable types that will
> remain top-level constructs in the back-end.
>
> Bug: skia:12670
> Change-Id: I73e69cecf6237a1c1180ad38d9b5d52ea80316fb
> Reviewed-on: https://skia-review.googlesource.com/c/skia/+/474218
> Commit-Queue: Brian Osman <brianosman@google.com>
> Reviewed-by: Ethan Nicholas <ethannicholas@google.com>

Bug: skia:12670
Change-Id: I01c0323bba7ce0bddea5f9fb907e2b60e6b812d2
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/475156
Bot-Commit: Rubber Stamper <rubber-stamper@appspot.gserviceaccount.com>
Commit-Queue: Brian Osman <brianosman@google.com>

resources/sksl/errors/VertexEarlyReturn.vert

@@ -1,4 +1,4 @@
-layout(set=0) uniform half zoom;
+uniform half zoom;
 
 void main() {
     sk_Position = half4(1);

resources/sksl/metal/OpaqueTypeInStruct.sksl

@@ -1,3 +1,3 @@
 struct Bad { sampler x; };
-layout (set=0) uniform Bad b;
+uniform Bad b;
 void main() {}

resources/sksl/shared/ComplexDelete.sksl

@@ -1,4 +1,4 @@
-layout(set=0) uniform float4x4 colorXform;
+uniform float4x4 colorXform;
 layout(binding=0) uniform sampler2D s;
 
 void main() {

resources/sksl/shared/NoFragCoordsPosRT.vert

@@ -1,6 +1,6 @@
 /*#pragma settings CannotUseFragCoord*/
 
-layout(set=0) uniform float4 sk_RTAdjust;
+uniform float4 sk_RTAdjust;
 layout(location=0) in float4 pos;
 
 void main() {

resources/sksl/shared/NormalizationVert.vert

@@ -1,4 +1,4 @@
-layout(set=0) uniform float4 sk_RTAdjust;
+uniform float4 sk_RTAdjust;
 
 void main() {
     sk_Position = half4(1);

src/sksl/SkSLDehydrator.cpp

@@ -194,6 +194,7 @@ void Dehydrator::write(const Symbol& s) {
                         this->write(f.fName);
                         this->write(*f.fType);
                     }
+                    this->writeU8(t.isInterfaceBlock());
                     break;
                 default:
                     this->writeCommand(Rehydrator::kSystemType_Command);

src/sksl/SkSLRehydrator.cpp

@@ -191,9 +191,10 @@ const Symbol* Rehydrator::symbol() {
                 const Type* type = this->type();
                 fields.emplace_back(m, fieldName, type);
             }
+            bool interfaceBlock = this->readU8();
             skstd::string_view nameChars(*fSymbolTable->takeOwnershipOfString(std::move(name)));
-            const Type* result = fSymbolTable->takeOwnershipOfSymbol(
-                    Type::MakeStructType(/*line=*/-1, nameChars, std::move(fields)));
+            const Type* result = fSymbolTable->takeOwnershipOfSymbol(Type::MakeStructType(
+                    /*line=*/-1, nameChars, std::move(fields), interfaceBlock));
             this->addSymbol(id, result);
             return result;
         }

src/sksl/codegen/SkSLSPIRVCodeGenerator.cpp

@@ -2980,6 +2980,9 @@ void SPIRVCodeGenerator::writeLayout(const Layout& layout, SpvId target) {
 }
 
 void SPIRVCodeGenerator::writeLayout(const Layout& layout, SpvId target, int member) {
+    // 'binding' and 'set' can not be applied to struct members
+    SkASSERT(layout.fBinding == -1);
+    SkASSERT(layout.fSet == -1);
     if (layout.fLocation >= 0) {
         this->writeInstruction(SpvOpMemberDecorate, target, member, SpvDecorationLocation,
                                layout.fLocation, fDecorationBuffer);
@@ -3032,8 +3035,9 @@ SpvId SPIRVCodeGenerator::writeInterfaceBlock(const InterfaceBlock& intf, bool a
                             fContext.fTypes.fFloat2.get());
         {
             AutoAttachPoolToThread attach(fProgram.fPool.get());
-            const Type* rtFlipStructType = fProgram.fSymbols->takeOwnershipOfSymbol(
-                    Type::MakeStructType(type.fLine, type.name(), std::move(fields)));
+            const Type* rtFlipStructType =
+                    fProgram.fSymbols->takeOwnershipOfSymbol(Type::MakeStructType(
+                            type.fLine, type.name(), std::move(fields), /*interfaceBlock=*/true));
             const Variable* modifiedVar = fProgram.fSymbols->takeOwnershipOfSymbol(
                     std::make_unique<Variable>(intfVar.fLine,
                                                &intfVar.modifiers(),
@@ -3432,8 +3436,8 @@ void SPIRVCodeGenerator::writeUniformBuffer(std::shared_ptr<SymbolTable> topLeve
         fTopLevelUniformMap[var] = (int)fields.size();
         fields.emplace_back(var->modifiers(), var->name(), &var->type());
     }
-    fUniformBuffer.fStruct = Type::MakeStructType(/*line=*/-1, kUniformBufferName,
-                                                 std::move(fields));
+    fUniformBuffer.fStruct = Type::MakeStructType(
+            /*line=*/-1, kUniformBufferName, std::move(fields), /*interfaceBlock=*/true);
 
     // Create a global variable to contain this struct.
     Layout layout;
@@ -3477,8 +3481,8 @@ void SPIRVCodeGenerator::addRTFlipUniform(int line) {
                         SKSL_RTFLIP_NAME,
                         fContext.fTypes.fFloat2.get());
     skstd::string_view name = "sksl_synthetic_uniforms";
-    const Type* intfStruct =
-            fSynthetics.takeOwnershipOfSymbol(Type::MakeStructType(/*line=*/-1, name, fields));
+    const Type* intfStruct = fSynthetics.takeOwnershipOfSymbol(
+            Type::MakeStructType(/*line=*/-1, name, fields, /*interfaceBlock=*/true));
     int binding = fProgram.fConfig->fSettings.fRTFlipBinding;
     if (binding == -1) {
         fContext.fErrors->error(line, "layout(binding=...) is required in SPIR-V");

src/sksl/dsl/DSLCore.cpp

@@ -251,8 +251,9 @@ public:
             skslFields.push_back(SkSL::Type::Field(field.fModifiers.fModifiers, field.fName,
                                                    &field.fType.skslType()));
         }
-        const SkSL::Type* structType = ThreadContext::SymbolTable()->takeOwnershipOfSymbol(
-                SkSL::Type::MakeStructType(pos.line(), typeName, std::move(skslFields)));
+        const SkSL::Type* structType =
+                ThreadContext::SymbolTable()->takeOwnershipOfSymbol(SkSL::Type::MakeStructType(
+                        pos.line(), typeName, std::move(skslFields), /*interfaceBlock=*/true));
         DSLType varType = arraySize > 0 ? Array(structType, arraySize) : DSLType(structType);
         DSLGlobalVar var(modifiers, varType, !varName.empty() ? varName : typeName, DSLExpression(),
                 pos);

src/sksl/generated/sksl_vert.dehydrated.sksl

@@ -15,7 +15,7 @@ static uint8_t SKSL_INCLUDE_sksl_vert[] = {82,0,
 49,2,0,27,0,
 36,
 35,0,2,0,0,255,255,255,255,255,1,0,255,0,34,0,
-49,3,0,47,0,
+49,3,0,47,0,1,
 52,4,0,
 36,
 16,32,2,0,

src/sksl/ir/SkSLInterfaceBlock.h

@@ -31,15 +31,22 @@ class InterfaceBlock final : public ProgramElement {
 public:
     inline static constexpr Kind kProgramElementKind = Kind::kInterfaceBlock;
 
-    InterfaceBlock(int line, const Variable& var, skstd::string_view typeName,
-                   skstd::string_view instanceName, int arraySize,
+    InterfaceBlock(int line,
+                   const Variable& var,
+                   skstd::string_view typeName,
+                   skstd::string_view instanceName,
+                   int arraySize,
                    std::shared_ptr<SymbolTable> typeOwner)
-    : INHERITED(line, kProgramElementKind)
-    , fVariable(var)
-    , fTypeName(typeName)
-    , fInstanceName(instanceName)
-    , fArraySize(arraySize)
-    , fTypeOwner(std::move(typeOwner)) {}
+            : INHERITED(line, kProgramElementKind)
+            , fVariable(var)
+            , fTypeName(typeName)
+            , fInstanceName(instanceName)
+            , fArraySize(arraySize)
+            , fTypeOwner(std::move(typeOwner)) {
+        SkASSERT(fVariable.type().isInterfaceBlock() ||
+                 (fVariable.type().isArray() &&
+                  fVariable.type().componentType().isInterfaceBlock()));
+    }
 
     const Variable& variable() const {
         return fVariable;

src/sksl/ir/SkSLType.cpp

@@ -329,9 +329,10 @@ class StructType final : public Type {
 public:
     inline static constexpr TypeKind kTypeKind = TypeKind::kStruct;
 
-    StructType(int line, skstd::string_view name, std::vector<Field> fields)
+    StructType(int line, skstd::string_view name, std::vector<Field> fields, bool interfaceBlock)
         : INHERITED(std::move(name), "S", kTypeKind, line)
-        , fFields(std::move(fields)) {}
+        , fFields(std::move(fields))
+        , fInterfaceBlock(interfaceBlock) {}
 
     const std::vector<Field>& fields() const override {
         return fFields;
@@ -341,6 +342,10 @@ public:
         return true;
     }
 
+    bool isInterfaceBlock() const override {
+        return fInterfaceBlock;
+    }
+
     bool isPrivate() const override {
         return std::any_of(fFields.begin(), fFields.end(), [](const Field& f) {
             return f.fType->isPrivate();
@@ -365,6 +370,7 @@ private:
     using INHERITED = Type;
 
     std::vector<Field> fFields;
+    bool fInterfaceBlock;
 };
 
 class VectorType final : public Type {
@@ -456,8 +462,8 @@ std::unique_ptr<Type> Type::MakeScalarType(skstd::string_view name, const char*
 }
 
 std::unique_ptr<Type> Type::MakeStructType(int line, skstd::string_view name,
-                                           std::vector<Field> fields) {
-    return std::make_unique<StructType>(line, name, std::move(fields));
+                                           std::vector<Field> fields, bool interfaceBlock) {
+    return std::make_unique<StructType>(line, name, std::move(fields), interfaceBlock);
 }
 
 std::unique_ptr<Type> Type::MakeTextureType(const char* name, SpvDim_ dimensions, bool isDepth,
@@ -735,7 +741,8 @@ const Type* Type::clone(SymbolTable* symbolTable) const {
         }
         case TypeKind::kStruct: {
             const String* name = symbolTable->takeOwnershipOfString(String(this->name()));
-            return symbolTable->add(Type::MakeStructType(this->fLine, *name, this->fields()));
+            return symbolTable->add(Type::MakeStructType(
+                    this->fLine, *name, this->fields(), this->isInterfaceBlock()));
         }
         default:
             SkDEBUGFAILF("don't know how to clone type '%s'", this->description().c_str());

src/sksl/ir/SkSLType.h

@@ -141,8 +141,10 @@ public:
                                                  Type::TypeKind typeKind);
 
     /** Creates a struct type with the given fields. */
-    static std::unique_ptr<Type> MakeStructType(int line, skstd::string_view name,
-                                                std::vector<Field> fields);
+    static std::unique_ptr<Type> MakeStructType(int line,
+                                                skstd::string_view name,
+                                                std::vector<Field> fields,
+                                                bool interfaceBlock = false);
 
     /** Create a texture type. */
     static std::unique_ptr<Type> MakeTextureType(const char* name, SpvDim_ dimensions,
@@ -447,6 +449,10 @@ public:
         return false;
     }
 
+    virtual bool isInterfaceBlock() const {
+        return false;
+    }
+
     // Is this type something that can be bound & sampled from an SkRuntimeEffect?
     // Includes types that represent stages of the Skia pipeline (colorFilter, shader, blender).
     bool isEffectChild() const {

src/sksl/ir/SkSLVarDeclarations.cpp

@@ -97,7 +97,19 @@ void VarDeclaration::ErrorCheck(const Context& context,
                      Modifiers::kFlat_Flag | Modifiers::kNoPerspective_Flag;
     }
     // TODO(skbug.com/11301): Migrate above checks into building a mask of permitted layout flags
-    modifiers.checkPermitted(context, line, permitted, /*permittedLayoutFlags=*/~0);
+
+    int permittedLayoutFlags = ~0;
+    // We don't allow 'binding' or 'set' on normal uniform variables, only on textures, samplers,
+    // and interface blocks (holding uniform variables).
+    bool permitBindingAndSet = baseType->typeKind() == Type::TypeKind::kSampler ||
+                               baseType->typeKind() == Type::TypeKind::kSeparateSampler ||
+                               baseType->typeKind() == Type::TypeKind::kTexture ||
+                               baseType->isInterfaceBlock();
+    if ((modifiers.fFlags & Modifiers::kUniform_Flag) && !permitBindingAndSet) {
+        permittedLayoutFlags &= ~Layout::kBinding_Flag;
+        permittedLayoutFlags &= ~Layout::kSet_Flag;
+    }
+    modifiers.checkPermitted(context, line, permitted, permittedLayoutFlags);
 }
 
 bool VarDeclaration::ErrorCheckAndCoerce(const Context& context, const Variable& var,

tests/sksl/shared/ComplexDelete.glsl

@@ -1,6 +1,6 @@
 
 out vec4 sk_FragColor;
-layout (set = 0) uniform mat4 colorXform;
+uniform mat4 colorXform;
 layout (binding = 0) uniform sampler2D s;
 void main() {
     vec4 tmpColor;

tests/sksl/shared/NoFragCoordsPosRT.glsl

@@ -1,6 +1,6 @@
 #version 400
 out vec4 sk_FragCoord_Workaround;
-layout (set = 0) uniform vec4 sk_RTAdjust;
+uniform vec4 sk_RTAdjust;
 layout (location = 0) in vec4 pos;
 void main() {
     sk_FragCoord_Workaround = (gl_Position = pos);

tests/sksl/shared/NormalizationVert.glsl

@@ -1,5 +1,5 @@
 
-layout (set = 0) uniform vec4 sk_RTAdjust;
+uniform vec4 sk_RTAdjust;
 void main() {
     gl_Position = vec4(1.0);
     gl_Position = vec4(gl_Position.xy * sk_RTAdjust.xz + gl_Position.ww * sk_RTAdjust.yw, 0.0, gl_Position.w);