Reland "Restrict where 'binding' and 'set' can appear"
This is a reland of 9372ef0228be737c562122f93de057c11d170cfe
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>
diff --git a/resources/sksl/errors/VertexEarlyReturn.vert b/resources/sksl/errors/VertexEarlyReturn.vert
index a9fa7a4..adaebc9 100644
--- a/resources/sksl/errors/VertexEarlyReturn.vert
+++ b/resources/sksl/errors/VertexEarlyReturn.vert
@@ -1,4 +1,4 @@
-layout(set=0) uniform half zoom;
+uniform half zoom;
void main() {
sk_Position = half4(1);
diff --git a/resources/sksl/metal/OpaqueTypeInStruct.sksl b/resources/sksl/metal/OpaqueTypeInStruct.sksl
index bf5de70..cd18704 100644
--- a/resources/sksl/metal/OpaqueTypeInStruct.sksl
+++ b/resources/sksl/metal/OpaqueTypeInStruct.sksl
@@ -1,3 +1,3 @@
struct Bad { sampler x; };
-layout (set=0) uniform Bad b;
+uniform Bad b;
void main() {}
diff --git a/resources/sksl/shared/ComplexDelete.sksl b/resources/sksl/shared/ComplexDelete.sksl
index 5e5d686..8defd63 100644
--- a/resources/sksl/shared/ComplexDelete.sksl
+++ b/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() {
diff --git a/resources/sksl/shared/NoFragCoordsPosRT.vert b/resources/sksl/shared/NoFragCoordsPosRT.vert
index 49796a1..72b10bd 100644
--- a/resources/sksl/shared/NoFragCoordsPosRT.vert
+++ b/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() {
diff --git a/resources/sksl/shared/NormalizationVert.vert b/resources/sksl/shared/NormalizationVert.vert
index d919fef..4fd7c5c 100644
--- a/resources/sksl/shared/NormalizationVert.vert
+++ b/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);
diff --git a/src/sksl/SkSLDehydrator.cpp b/src/sksl/SkSLDehydrator.cpp
index cc1880e..80036e7 100644
--- a/src/sksl/SkSLDehydrator.cpp
+++ b/src/sksl/SkSLDehydrator.cpp
@@ -194,6 +194,7 @@
this->write(f.fName);
this->write(*f.fType);
}
+ this->writeU8(t.isInterfaceBlock());
break;
default:
this->writeCommand(Rehydrator::kSystemType_Command);
diff --git a/src/sksl/SkSLRehydrator.cpp b/src/sksl/SkSLRehydrator.cpp
index 9722171..d73258d 100644
--- a/src/sksl/SkSLRehydrator.cpp
+++ b/src/sksl/SkSLRehydrator.cpp
@@ -191,9 +191,10 @@
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;
}
diff --git a/src/sksl/codegen/SkSLSPIRVCodeGenerator.cpp b/src/sksl/codegen/SkSLSPIRVCodeGenerator.cpp
index 73f6aa9..5282b1c 100644
--- a/src/sksl/codegen/SkSLSPIRVCodeGenerator.cpp
+++ b/src/sksl/codegen/SkSLSPIRVCodeGenerator.cpp
@@ -2980,6 +2980,9 @@
}
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 @@
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 @@
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 @@
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");
diff --git a/src/sksl/dsl/DSLCore.cpp b/src/sksl/dsl/DSLCore.cpp
index 0bc0a7d..347ffa3 100644
--- a/src/sksl/dsl/DSLCore.cpp
+++ b/src/sksl/dsl/DSLCore.cpp
@@ -251,8 +251,9 @@
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);
diff --git a/src/sksl/generated/sksl_vert.dehydrated.sksl b/src/sksl/generated/sksl_vert.dehydrated.sksl
index b4a1315..5c57820 100644
--- a/src/sksl/generated/sksl_vert.dehydrated.sksl
+++ b/src/sksl/generated/sksl_vert.dehydrated.sksl
@@ -15,7 +15,7 @@
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,
diff --git a/src/sksl/ir/SkSLInterfaceBlock.h b/src/sksl/ir/SkSLInterfaceBlock.h
index 53eb1fc..f5331ad 100644
--- a/src/sksl/ir/SkSLInterfaceBlock.h
+++ b/src/sksl/ir/SkSLInterfaceBlock.h
@@ -31,15 +31,22 @@
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;
diff --git a/src/sksl/ir/SkSLType.cpp b/src/sksl/ir/SkSLType.cpp
index 6f2f4d7..8fc9d4d 100644
--- a/src/sksl/ir/SkSLType.cpp
+++ b/src/sksl/ir/SkSLType.cpp
@@ -329,9 +329,10 @@
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 @@
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 @@
using INHERITED = Type;
std::vector<Field> fFields;
+ bool fInterfaceBlock;
};
class VectorType final : public Type {
@@ -456,8 +462,8 @@
}
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 @@
}
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());
diff --git a/src/sksl/ir/SkSLType.h b/src/sksl/ir/SkSLType.h
index 6529753..afd4940 100644
--- a/src/sksl/ir/SkSLType.h
+++ b/src/sksl/ir/SkSLType.h
@@ -141,8 +141,10 @@
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 @@
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 {
diff --git a/src/sksl/ir/SkSLVarDeclarations.cpp b/src/sksl/ir/SkSLVarDeclarations.cpp
index c4643d8..38efc05 100644
--- a/src/sksl/ir/SkSLVarDeclarations.cpp
+++ b/src/sksl/ir/SkSLVarDeclarations.cpp
@@ -97,7 +97,19 @@
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,
diff --git a/tests/sksl/shared/ComplexDelete.glsl b/tests/sksl/shared/ComplexDelete.glsl
index e54be88..d5aea34 100644
--- a/tests/sksl/shared/ComplexDelete.glsl
+++ b/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;
diff --git a/tests/sksl/shared/NoFragCoordsPosRT.glsl b/tests/sksl/shared/NoFragCoordsPosRT.glsl
index 8858cbe..5192ea4 100644
--- a/tests/sksl/shared/NoFragCoordsPosRT.glsl
+++ b/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);
diff --git a/tests/sksl/shared/NormalizationVert.glsl b/tests/sksl/shared/NormalizationVert.glsl
index 90be4e5..926f988 100644
--- a/tests/sksl/shared/NormalizationVert.glsl
+++ b/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);