Revert "Add support for top-level uniforms in SPIR-V."
This reverts commit acba30420ce27b3464fd04fac90e822ffd0245d8.
Reason for revert: ASAN breakage on tree for
Perf-Win2019-Clang-GCE-CPU-AVX2-x86_64-(Debug|Release)-All-ASAN
Address 0x009af69fda78 is located in stack of thread T0 at offset 1272 in frame
#0 0x7ff75c069ddf in _asan_wrap_RtlReAllocateHeap+0x44014f (c:\b\s\w\ir\build\nanobench.exe+0x1413a9ddf)
This frame has 35 object(s):
[32, 104) 'body' (line 3363)
[144, 152) 'main' (line 3365)
[176, 184) 'ref.tmp' (line 3366)
[208, 240) '__begin1' (line 3366)
[272, 304) '__end1' (line 3366)
[336, 344) 'ref.tmp27' (line 3370)
[368, 384) 'ref.tmp31' (line 3371)
[400, 416) 'interfaceVars' (line 3382)
[432, 440) 'ref.tmp48' (line 3383)
[464, 496) '__begin151' (line 3383)
[528, 560) '__end154' (line 3383)
[592, 596) 'id' (line 3386)
[608, 624) 'tmp' (line 3393)
[640, 648) 'ref.tmp114' (line 3398)
[672, 704) '__begin1117' (line 3398)
[736, 768) '__end1120' (line 3398)
[800, 1008) 'uniformBuffer' (line 3405)
[1072, 1280) 'ref.tmp159' (line 3407) <== Memory access at offset 1272 is inside this variable
[1344, 1360) 'agg.tmp'
[1376, 1576) 'adapter' (line 3411)
[1648, 1848) 'ref.tmp179' (line 3413)
[1920, 1928) 'ref.tmp191' (line 3415)
[1952, 1960) 'ref.tmp210' (line 3421)
[1984, 2016) '__begin1213' (line 3421)
[2048, 2080) '__end1216' (line 3421)
[2112, 2120) '__begin1242' (line 3427)
[2144, 2152) '__end1247' (line 3427)
[2176, 2192) 'entry256' (line 3427)
[2208, 2224) 'tmp298' (line 3433)
[2240, 2256) 'agg.tmp307'
[2272, 2280) '__begin1365' (line 3457)
[2304, 2312) 'ref.tmp415' (line 3469)
[2336, 2368) '__begin1418' (line 3469)
[2400, 2432) '__end1421' (line 3469)
[2464, 2480) 'agg.tmp436'
Original change's description:
> Add support for top-level uniforms in SPIR-V.
>
> Previously, a uniform not wrapped in an interface block would report a
> SPIR-V error:
>
> "Variables identified with the Uniform storage class are
> used to access transparent buffer backed resources. Such variables must
> be typed as OpTypeStruct, or an array of this type..."
>
> Now, the SPIR-V code generator automatically detects such global
> variables and synthesizes a struct named _UniformBuffer to hold them.
> When these variables are accessed, an OpAccessChain instruction is added
> to grab the variable out of the struct.
>
> Change-Id: I5e852d4de01b866c291506cc8cf6eb547f097d66
> Bug: skia:11225
> Reviewed-on: https://skia-review.googlesource.com/c/skia/+/360776
> Commit-Queue: John Stiles <johnstiles@google.com>
> Reviewed-by: Brian Osman <brianosman@google.com>
> Auto-Submit: John Stiles <johnstiles@google.com>
TBR=brianosman@google.com,ethannicholas@google.com,johnstiles@google.com
Change-Id: Ib72e33dbd662a245c20bc9d45d1397454c9588a3
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Bug: skia:11225
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/362057
Reviewed-by: John Stiles <johnstiles@google.com>
Commit-Queue: John Stiles <johnstiles@google.com>
Auto-Submit: John Stiles <johnstiles@google.com>
diff --git a/src/sksl/SkSLSPIRVCodeGenerator.cpp b/src/sksl/SkSLSPIRVCodeGenerator.cpp
index 1d8848c..875ae0f 100644
--- a/src/sksl/SkSLSPIRVCodeGenerator.cpp
+++ b/src/sksl/SkSLSPIRVCodeGenerator.cpp
@@ -1742,7 +1742,7 @@
case Expression::Kind::kFieldAccess: {
const FieldAccess& fieldExpr = expr.as<FieldAccess>();
chain = this->getAccessChain(*fieldExpr.base(), out);
- IntLiteral index(fContext, /*offset=*/-1, fieldExpr.fieldIndex());
+ IntLiteral index(fContext, -1, fieldExpr.fieldIndex());
chain.push_back(this->writeIntLiteral(index));
break;
}
@@ -1867,30 +1867,14 @@
const SPIRVCodeGenerator::Precision fPrecision;
};
-int SPIRVCodeGenerator::findUniformFieldIndex(const Variable& var) const {
- auto iter = fTopLevelUniformMap.find(&var);
- return (iter != fTopLevelUniformMap.end()) ? iter->second : -1;
-}
-
std::unique_ptr<SPIRVCodeGenerator::LValue> SPIRVCodeGenerator::getLValue(const Expression& expr,
OutputStream& out) {
const Type& type = expr.type();
Precision precision = type.highPrecision() ? Precision::kHigh : Precision::kLow;
switch (expr.kind()) {
case Expression::Kind::kVariableReference: {
- const Variable& var = *expr.as<VariableReference>().variable();
- int uniformIdx = this->findUniformFieldIndex(var);
- if (uniformIdx >= 0) {
- IntLiteral uniformIdxLiteral{fContext, /*offset=*/-1, uniformIdx};
- SpvId memberId = this->nextId();
- SpvId typeId = this->getPointerType(type, SpvStorageClassUniform);
- SpvId uniformIdxId = this->writeIntLiteral(uniformIdxLiteral);
- this->writeInstruction(SpvOpAccessChain, typeId, memberId, fUniformBufferId,
- uniformIdxId, out);
- return std::make_unique<PointerLValue>(*this, memberId, this->getType(type),
- precision);
- }
SpvId typeId;
+ const Variable& var = *expr.as<VariableReference>().variable();
if (var.modifiers().fLayout.fBuiltin == SK_IN_BUILTIN) {
typeId = this->getType(*Type::MakeArrayType("sk_in", var.type().componentType(),
fSkInCount));
@@ -1971,21 +1955,13 @@
}
SpvId SPIRVCodeGenerator::writeVariableReference(const VariableReference& ref, OutputStream& out) {
- // Is this variable is actually a uniform at global scope?
- const Variable* variable = ref.variable();
- if (this->findUniformFieldIndex(*variable) >= 0) {
- // SPIR-V doesn't allow uniforms at global scope, so we've stashed them in an interface
- // block. We need to fetch it from there. getLValue knows how to do this.
- return this->getLValue(ref, out)->load(out);
- }
-
SpvId result = this->nextId();
- auto entry = fVariableMap.find(variable);
+ auto entry = fVariableMap.find(ref.variable());
SkASSERT(entry != fVariableMap.end());
SpvId var = entry->second;
- this->writeInstruction(SpvOpLoad, this->getType(variable->type()), result, var, out);
- this->writePrecisionModifier(variable->type(), result);
- if (variable->modifiers().fLayout.fBuiltin == SK_FRAGCOORD_BUILTIN &&
+ this->writeInstruction(SpvOpLoad, this->getType(ref.variable()->type()), result, var, out);
+ this->writePrecisionModifier(ref.variable()->type(), result);
+ if (ref.variable()->modifiers().fLayout.fBuiltin == SK_FRAGCOORD_BUILTIN &&
fProgram.fSettings.fFlipY) {
// The x component never changes, so just grab it
SpvId xId = this->nextId();
@@ -2007,14 +1983,11 @@
fErrors.error(ref.fOffset, "RTHeightOffset is negative");
}
fields.emplace_back(
- Modifiers(Layout(/*flags=*/0, /*location=*/-1,
- fProgram.fSettings.fRTHeightOffset,
- /*binding=*/-1, /*index=*/-1, /*set=*/-1, /*builtin=*/-1,
- /*inputAttachmentIndex=*/-1, Layout::Format::kUnspecified,
- Layout::kUnspecified_Primitive, /*maxVertices=*/1,
- /*invocations=*/-1, /*marker=*/"", /*when=*/"",
+ Modifiers(Layout(0, -1, fProgram.fSettings.fRTHeightOffset, -1, -1, -1, -1,
+ -1, Layout::Format::kUnspecified,
+ Layout::kUnspecified_Primitive, 1, -1, "", "",
Layout::kNo_Key, Layout::CType::kDefault),
- /*flags=*/0),
+ 0),
SKSL_RTHEIGHT_NAME, fContext.fTypes.fFloat.get());
StringFragment name("sksl_synthetic_uniforms");
std::unique_ptr<Type> intfStruct = Type::MakeStructType(/*offset=*/-1, name,
@@ -2028,14 +2001,11 @@
fErrors.error(ref.fOffset, "layout(set=...) is required in SPIR-V");
}
bool usePushConstants = fProgram.fSettings.fUsePushConstants;
- int flags = usePushConstants ? Layout::Flag::kPushConstant_Flag : 0;
- Modifiers modifiers(
- Layout(flags, /*location=*/-1, /*offset=*/-1, binding, /*index=*/-1,
- set, /*builtin=*/-1, /*inputAttachmentIndex=*/-1,
- Layout::Format::kUnspecified, Layout::kUnspecified_Primitive,
- /*maxVertices=*/-1, /*invocations=*/-1, /*marker=*/"", /*when=*/"",
- Layout::kNo_Key, Layout::CType::kDefault),
- Modifiers::kUniform_Flag);
+ Layout layout;
+ layout.fFlags = usePushConstants ? Layout::Flag::kPushConstant_Flag : 0;
+ layout.fBinding = binding;
+ layout.fSet = set;
+ Modifiers modifiers(layout, Modifiers::kUniform_Flag);
const Variable* intfVar = fSynthetics.takeOwnershipOfSymbol(
std::make_unique<Variable>(/*offset=*/-1,
fProgram.fModifiers->addToPool(modifiers),
@@ -2097,7 +2067,7 @@
return adjusted;
}
- if (variable->modifiers().fLayout.fBuiltin == SK_CLOCKWISE_BUILTIN &&
+ if (ref.variable()->modifiers().fLayout.fBuiltin == SK_CLOCKWISE_BUILTIN &&
!fProgram.fSettings.fFlipY) {
// FrontFacing in Vulkan is defined in terms of a top-down render target. In skia, we use
// the default convention of "counter-clockwise face is front".
@@ -2912,7 +2882,8 @@
return var.modifiers().fLayout.fBuiltin == SK_SAMPLEMASK_BUILTIN;
}
-void SPIRVCodeGenerator::writeGlobalVar(Program::Kind kind, const VarDeclaration& varDecl) {
+void SPIRVCodeGenerator::writeGlobalVar(Program::Kind kind, const VarDeclaration& varDecl,
+ OutputStream& out) {
const Variable& var = varDecl.var();
// These haven't been implemented in our SPIR-V generator yet and we only currently use them
// in the OpenGL backend.
@@ -2935,15 +2906,11 @@
if (is_dead(var, fProgram.fUsage.get())) {
return;
}
- SpvStorageClass_ storageClass = get_storage_class(var, SpvStorageClassPrivate);
- if (storageClass == SpvStorageClassUniform) {
- // Top-level uniforms are emitted in writeUniformBuffer.
- fTopLevelUniforms.push_back(&varDecl);
- return;
- }
const Type& type = var.type();
+ SpvStorageClass_ storageClass = get_storage_class(var, SpvStorageClassPrivate);
Layout layout = var.modifiers().fLayout;
- if (layout.fSet < 0 && storageClass == SpvStorageClassUniformConstant) {
+ if (layout.fSet < 0 && (storageClass == SpvStorageClassUniform ||
+ storageClass == SpvStorageClassUniformConstant)) {
layout.fSet = fProgram.fSettings.fDefaultUniformSet;
}
SpvId id = this->nextId();
@@ -3260,18 +3227,14 @@
invocations, out);
}
-// Given any function, returns the top-level symbol table (OUTSIDE of the function's scope).
-static std::shared_ptr<SymbolTable> get_top_level_symbol_table(const FunctionDeclaration& anyFunc) {
- return anyFunc.definition()->body()->as<Block>().symbolTable()->fParent;
-}
-
SPIRVCodeGenerator::EntrypointAdapter SPIRVCodeGenerator::writeEntrypointAdapter(
const FunctionDeclaration& main) {
// Our goal is to synthesize a tiny helper function which looks like this:
// void _entrypoint() { sk_FragColor = main(); }
// Fish a symbol table out of main().
- std::shared_ptr<SymbolTable> symbolTable = get_top_level_symbol_table(main);
+ std::shared_ptr<SymbolTable> symbolTable =
+ main.definition()->body()->as<Block>().symbolTable()->fParent;
// Get `sk_FragColor` as a writable reference.
const Symbol* skFragColorSymbol = (*symbolTable)["sk_FragColor"];
@@ -3322,45 +3285,6 @@
return adapter;
}
-SPIRVCodeGenerator::UniformBuffer SPIRVCodeGenerator::writeUniformBuffer(
- std::shared_ptr<SymbolTable> topLevelSymbolTable) {
- SkASSERT(!fTopLevelUniforms.empty());
- static constexpr char kUniformBufferName[] = "_UniformBuffer";
-
- SPIRVCodeGenerator::UniformBuffer uniformBuffer;
-
- // Convert the list of top-level uniforms into a matching struct named _UniformBuffer, and build
- // a lookup table of variables to UniformBuffer field indices.
- std::vector<Type::Field> fields;
- fields.reserve(fTopLevelUniforms.size());
- fTopLevelUniformMap.reserve(fTopLevelUniforms.size());
- for (const VarDeclaration* topLevelUniform : fTopLevelUniforms) {
- const Variable* var = &topLevelUniform->var();
- fTopLevelUniformMap[var] = (int)fields.size();
- fields.emplace_back(var->modifiers(), var->name(), &var->type());
- }
- uniformBuffer.fStruct = Type::MakeStructType(/*offset=*/-1, kUniformBufferName,
- std::move(fields));
-
- // Create a global variable to contain this struct.
- uniformBuffer.fLayout.fBinding = fProgram.fSettings.fDefaultUniformBinding;
- uniformBuffer.fLayout.fSet = fProgram.fSettings.fDefaultUniformSet;
- uniformBuffer.fModifiers = Modifiers{uniformBuffer.fLayout, Modifiers::kUniform_Flag};
- uniformBuffer.fInnerVariable = std::make_unique<Variable>(
- /*offset=*/-1, &uniformBuffer.fModifiers, kUniformBufferName,
- uniformBuffer.fStruct.get(), /*builtin=*/false, Variable::Storage::kGlobal);
-
- // Create an interface block object for this global variable.
- uniformBuffer.fInterfaceBlock = std::make_unique<InterfaceBlock>(
- /*offset=*/-1, uniformBuffer.fInnerVariable.get(), kUniformBufferName,
- kUniformBufferName, /*arraySize=*/0, topLevelSymbolTable);
-
- // Generate an interface block and hold onto its ID.
- fUniformBufferId = this->writeInterfaceBlock(*uniformBuffer.fInterfaceBlock);
-
- return uniformBuffer;
-}
-
void SPIRVCodeGenerator::writeInstructions(const Program& program, OutputStream& out) {
fGLSLExtendedInstructions = this->nextId();
StringStream body;
@@ -3401,14 +3325,10 @@
for (const ProgramElement* e : program.elements()) {
if (e->is<GlobalVarDeclaration>()) {
this->writeGlobalVar(program.fKind,
- e->as<GlobalVarDeclaration>().declaration()->as<VarDeclaration>());
+ e->as<GlobalVarDeclaration>().declaration()->as<VarDeclaration>(),
+ body);
}
}
- // Emit top-level uniforms into a dedicated uniform buffer.
- UniformBuffer uniformBuffer;
- if (!fTopLevelUniforms.empty()) {
- uniformBuffer = this->writeUniformBuffer(get_top_level_symbol_table(*main));
- }
// If main() returns a half4, synthesize a tiny entrypoint function which invokes the real
// main() and stores the result into sk_FragColor.
EntrypointAdapter adapter;
diff --git a/src/sksl/SkSLSPIRVCodeGenerator.h b/src/sksl/SkSLSPIRVCodeGenerator.h
index 990c57d..382b137 100644
--- a/src/sksl/SkSLSPIRVCodeGenerator.h
+++ b/src/sksl/SkSLSPIRVCodeGenerator.h
@@ -195,14 +195,12 @@
SpvId writeFunction(const FunctionDefinition& f, OutputStream& out);
- void writeGlobalVar(Program::Kind kind, const VarDeclaration& v);
+ void writeGlobalVar(Program::Kind kind, const VarDeclaration& v, OutputStream& out);
void writeVarDeclaration(const VarDeclaration& var, OutputStream& out);
SpvId writeVariableReference(const VariableReference& ref, OutputStream& out);
- int findUniformFieldIndex(const Variable& var) const;
-
std::unique_ptr<LValue> getLValue(const Expression& value, OutputStream& out);
SpvId writeExpression(const Expression& expr, OutputStream& out);
@@ -396,16 +394,6 @@
EntrypointAdapter writeEntrypointAdapter(const FunctionDeclaration& main);
- struct UniformBuffer {
- std::unique_ptr<InterfaceBlock> fInterfaceBlock;
- std::unique_ptr<Variable> fInnerVariable;
- std::unique_ptr<Type> fStruct;
- Layout fLayout;
- Modifiers fModifiers;
- };
-
- UniformBuffer writeUniformBuffer(std::shared_ptr<SymbolTable> topLevelSymbolTable);
-
const Context& fContext;
const MemoryLayout fDefaultLayout;
@@ -443,11 +431,6 @@
// holds variables synthesized during output, for lifetime purposes
SymbolTable fSynthetics;
int fSkInCount = 1;
- // Holds a list of uniforms that were declared as globals at the top-level instead of in an
- // interface block.
- std::vector<const VarDeclaration*> fTopLevelUniforms;
- std::unordered_map<const Variable*, int> fTopLevelUniformMap; //<var, UniformBuffer field index>
- SpvId fUniformBufferId = -1;
friend class PointerLValue;
friend class SwizzleLValue;
diff --git a/src/sksl/ir/SkSLType.h b/src/sksl/ir/SkSLType.h
index 61b3ccc..516e839 100644
--- a/src/sksl/ir/SkSLType.h
+++ b/src/sksl/ir/SkSLType.h
@@ -243,8 +243,7 @@
/**
* For matrices and vectors, returns the type of individual cells (e.g. mat2 has a component
- * type of Float). For arrays, returns the base type. For all other types, returns the type
- * itself.
+ * type of Float). For all other types, returns the type itself.
*/
const Type& componentType() const {
if (fComponentType) {