From e051cd35e4d575d04f99104603197ac5a3c41105 Mon Sep 17 00:00:00 2001 From: Brian Osman Date: Mon, 22 Nov 2021 12:51:49 -0500 Subject: [PATCH] Disallow 'binding' and 'set' on push constants These aren't allowed on push constants (it's a validation error now), so we at least catch it in the SPIRV generator and emit an error. Fixed two places where we were breaking this rule when automatically adjusting layouts for interface blocks. Bug: skia:12670 Change-Id: I08b88f4c2844da77239207817f49510118be4e60 Reviewed-on: https://skia-review.googlesource.com/c/skia/+/474976 Commit-Queue: Brian Osman Reviewed-by: Ethan Nicholas --- .../spirv/InterfaceBlockPushConstant.sksl | 2 +- src/sksl/codegen/SkSLSPIRVCodeGenerator.cpp | 53 +++++++++++-------- src/sksl/codegen/SkSLSPIRVCodeGenerator.h | 4 +- 3 files changed, 35 insertions(+), 24 deletions(-) diff --git a/resources/sksl/spirv/InterfaceBlockPushConstant.sksl b/resources/sksl/spirv/InterfaceBlockPushConstant.sksl index 2629617f88..8723963302 100644 --- a/resources/sksl/spirv/InterfaceBlockPushConstant.sksl +++ b/resources/sksl/spirv/InterfaceBlockPushConstant.sksl @@ -1,4 +1,4 @@ -layout(push_constant, binding=456) uniform testBlock { +layout(push_constant) uniform testBlock { layout(offset=16) half2x2 m1; layout(offset=32) half2x2 m2; }; diff --git a/src/sksl/codegen/SkSLSPIRVCodeGenerator.cpp b/src/sksl/codegen/SkSLSPIRVCodeGenerator.cpp index 5282b1c0a7..d419a9a3ed 100644 --- a/src/sksl/codegen/SkSLSPIRVCodeGenerator.cpp +++ b/src/sksl/codegen/SkSLSPIRVCodeGenerator.cpp @@ -498,7 +498,7 @@ void SPIRVCodeGenerator::writeStruct(const Type& type, const MemoryLayout& memor } } this->writeInstruction(SpvOpMemberName, resultId, i, field.fName, fNameBuffer); - this->writeLayout(fieldLayout, resultId, i); + this->writeFieldLayout(fieldLayout, resultId, i); if (field.fModifiers.fLayout.fBuiltin < 0) { this->writeInstruction(SpvOpMemberDecorate, resultId, (SpvId) i, SpvDecorationOffset, (SpvId) offset, fDecorationBuffer); @@ -2950,23 +2950,31 @@ SpvId SPIRVCodeGenerator::writeFunction(const FunctionDefinition& f, OutputStrea return result; } -void SPIRVCodeGenerator::writeLayout(const Layout& layout, SpvId target) { +void SPIRVCodeGenerator::writeLayout(const Layout& layout, SpvId target, int line) { bool isPushConstant = (layout.fFlags & Layout::kPushConstant_Flag); if (layout.fLocation >= 0) { this->writeInstruction(SpvOpDecorate, target, SpvDecorationLocation, layout.fLocation, fDecorationBuffer); } - if (layout.fBinding >= 0 && !isPushConstant) { - this->writeInstruction(SpvOpDecorate, target, SpvDecorationBinding, layout.fBinding, - fDecorationBuffer); + if (layout.fBinding >= 0) { + if (isPushConstant) { + fContext.fErrors->error(line, "Can't apply 'binding' to push constants"); + } else { + this->writeInstruction(SpvOpDecorate, target, SpvDecorationBinding, layout.fBinding, + fDecorationBuffer); + } } if (layout.fIndex >= 0) { this->writeInstruction(SpvOpDecorate, target, SpvDecorationIndex, layout.fIndex, fDecorationBuffer); } - if (layout.fSet >= 0 && !isPushConstant) { - this->writeInstruction(SpvOpDecorate, target, SpvDecorationDescriptorSet, layout.fSet, - fDecorationBuffer); + if (layout.fSet >= 0) { + if (isPushConstant) { + fContext.fErrors->error(line, "Can't apply 'set' to push constants"); + } else { + this->writeInstruction(SpvOpDecorate, target, SpvDecorationDescriptorSet, layout.fSet, + fDecorationBuffer); + } } if (layout.fInputAttachmentIndex >= 0) { this->writeInstruction(SpvOpDecorate, target, SpvDecorationInputAttachmentIndex, @@ -2979,7 +2987,7 @@ void SPIRVCodeGenerator::writeLayout(const Layout& layout, SpvId target) { } } -void SPIRVCodeGenerator::writeLayout(const Layout& layout, SpvId target, int member) { +void SPIRVCodeGenerator::writeFieldLayout(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); @@ -3069,10 +3077,10 @@ SpvId SPIRVCodeGenerator::writeInterfaceBlock(const InterfaceBlock& intf, bool a this->writeInstruction(SpvOpTypePointer, ptrType, storageClass, typeId, fConstantBuffer); this->writeInstruction(SpvOpVariable, ptrType, result, storageClass, fConstantBuffer); Layout layout = intfModifiers.fLayout; - if (intfModifiers.fFlags & Modifiers::kUniform_Flag && layout.fSet == -1) { - layout.fSet = 0; + if (storageClass == SpvStorageClassUniform && layout.fSet < 0) { + layout.fSet = fProgram.fConfig->fSettings.fDefaultUniformSet; } - this->writeLayout(layout, result); + this->writeLayout(layout, result, intfVar.fLine); fVariableMap[&intfVar] = result; return result; } @@ -3129,7 +3137,7 @@ void SPIRVCodeGenerator::writeGlobalVar(ProgramKind kind, const VarDeclaration& this->writeInstruction(SpvOpStore, id, value, fGlobalInitializersBuffer); fCurrentBlock = 0; } - this->writeLayout(layout, id); + this->writeLayout(layout, id, var.fLine); if (var.modifiers().fFlags & Modifiers::kFlat_Flag) { this->writeInstruction(SpvOpDecorate, id, SpvDecorationFlat, fDecorationBuffer); } @@ -3483,15 +3491,18 @@ void SPIRVCodeGenerator::addRTFlipUniform(int line) { skstd::string_view name = "sksl_synthetic_uniforms"; 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"); - } - int set = fProgram.fConfig->fSettings.fRTFlipSet; - if (set == -1) { - fContext.fErrors->error(line, "layout(set=...) is required in SPIR-V"); - } bool usePushConstants = fProgram.fConfig->fSettings.fUsePushConstants; + int binding = -1, set = -1; + if (!usePushConstants) { + binding = fProgram.fConfig->fSettings.fRTFlipBinding; + if (binding == -1) { + fContext.fErrors->error(line, "layout(binding=...) is required in SPIR-V"); + } + set = fProgram.fConfig->fSettings.fRTFlipSet; + if (set == -1) { + fContext.fErrors->error(line, "layout(set=...) is required in SPIR-V"); + } + } int flags = usePushConstants ? Layout::Flag::kPushConstant_Flag : 0; const Modifiers* modsPtr; { diff --git a/src/sksl/codegen/SkSLSPIRVCodeGenerator.h b/src/sksl/codegen/SkSLSPIRVCodeGenerator.h index 657e157eb4..cc59e3fc1b 100644 --- a/src/sksl/codegen/SkSLSPIRVCodeGenerator.h +++ b/src/sksl/codegen/SkSLSPIRVCodeGenerator.h @@ -201,9 +201,9 @@ private: std::vector getAccessChain(const Expression& expr, OutputStream& out); - void writeLayout(const Layout& layout, SpvId target); + void writeLayout(const Layout& layout, SpvId target, int line); - void writeLayout(const Layout& layout, SpvId target, int member); + void writeFieldLayout(const Layout& layout, SpvId target, int member); void writeStruct(const Type& type, const MemoryLayout& layout, SpvId resultId);