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

This reverts commit 9372ef0228.

Reason for revert: Unhappy bots

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: Ie518192d9a52fc896e615ec08ce0674ad683ec61
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/475099
Auto-Submit: Brian Osman <brianosman@google.com>
Commit-Queue: Rubber Stamper <rubber-stamper@appspot.gserviceaccount.com>
Bot-Commit: Rubber Stamper <rubber-stamper@appspot.gserviceaccount.com>
This commit is contained in:
Brian Osman 2021-11-22 19:51:10 +00:00 committed by SkCQ
parent e00afb0a1a
commit d7ebf8604e
17 changed files with 35 additions and 74 deletions

View File

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

View File

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

View File

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

View File

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

View File

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

View File

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

View File

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

View File

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

View File

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

View File

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

View File

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

View File

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

View File

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

View File

@ -97,19 +97,7 @@ void VarDeclaration::ErrorCheck(const Context& context,
Modifiers::kFlat_Flag | Modifiers::kNoPerspective_Flag; Modifiers::kFlat_Flag | Modifiers::kNoPerspective_Flag;
} }
// TODO(skbug.com/11301): Migrate above checks into building a mask of permitted layout flags // 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, bool VarDeclaration::ErrorCheckAndCoerce(const Context& context, const Variable& var,

View File

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

View File

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

View File

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