diff --git a/MobileGL/MG_Backend/DirectGLES/Managers.cpp b/MobileGL/MG_Backend/DirectGLES/Managers.cpp index f07b97c3..34153f10 100644 --- a/MobileGL/MG_Backend/DirectGLES/Managers.cpp +++ b/MobileGL/MG_Backend/DirectGLES/Managers.cpp @@ -5329,7 +5329,11 @@ namespace MobileGL::MG_Backend::DirectGLES { // 1D-array image comes out as ivec2(ivec2(u, layer), 0) - three components in a // two-component constructor, which every driver rejects, taking the whole // program with it. The pass does the conversion properly - type to 2D array, - // coordinate to (u, 0, layer) - before SPIRV-Cross can apply its own. + // coordinate to (u, 0, layer) - before SPIRV-Cross can apply its own. It also + // owns the NON-arrayed 1D storage image whenever the module performs an atomic on + // one: SPIRV-Cross widens the coordinate in OpImageRead and OpImageWrite but not + // in OpImageTexelPointer, so imageAtomic* alone came out with a scalar coordinate + // against an iimage2D and lost the stage. Vector arrayImageSpirv; if (MG_Util::ShaderTranspiler::ShaderCompiler::Lower1DArrayImagesForEssl(*effectiveSpirv, arrayImageSpirv, enableSpirvValidation) && diff --git a/MobileGL/MG_Test/Program/ProgramUtilTest.cpp b/MobileGL/MG_Test/Program/ProgramUtilTest.cpp index 8c82509e..0505a03d 100644 --- a/MobileGL/MG_Test/Program/ProgramUtilTest.cpp +++ b/MobileGL/MG_Test/Program/ProgramUtilTest.cpp @@ -3348,11 +3348,38 @@ namespace { return count; } + // Same word walk, for the NON-arrayed half of the family (Arrayed == 0). + SizeT Count1DNonArrayedStorageImageTypes(const Vector& spirv) { + constexpr unsigned kOpTypeImage = 25, kDim1D = 0; + SizeT count = 0; + for (SizeT i = 5; i < spirv.size();) { + const unsigned wordCount = spirv[i] >> 16; + const unsigned opcode = spirv[i] & 0xFFFFu; + if (wordCount == 0 || i + wordCount > spirv.size()) break; + if (opcode == kOpTypeImage && wordCount >= 8 && spirv[i + 3] == kDim1D && spirv[i + 5] == 0u && + spirv[i + 7] == 2u) { + ++count; + } + i += wordCount; + } + return count; + } + const char* k1DArrayImageCompute = R"(#version 440 core layout (local_size_x = 1) in; layout (location = 0, r32ui) readonly uniform uimage1DArray i0; layout (std430, binding = 0) buffer SSB { uint sum; } ssb; void main() { ssb.sum = imageLoad(i0, ivec2(2, 3)).r; } +)"; + + // KHR-GL4x.shader_image_load_store.basic-allTargets-atomic's own shape, minus the six other + // targets: a non-arrayed 1D storage image reached ONLY through an atomic. r32ui because ES + // defines image atomics on r32i/r32ui/r32f alone. + const char* k1DImageAtomicCompute = R"(#version 440 core +layout (local_size_x = 1) in; +layout (r32ui) coherent uniform uimage1D i0; +layout (std430, binding = 0) buffer SSB { uint sum; } ssb; +void main() { ssb.sum = imageAtomicAdd(i0, 2, 7u); } )"; } // namespace @@ -3464,9 +3491,11 @@ void main() { ssb.sum = imageLoad(i0, ivec2(2, 3)).r + imageLoad(i1, ivec3(1, 1, EXPECT_NE(essl.find("ivec3(2, 0, 3)"), String::npos) << essl; } -// Scope, half one: a NON-arrayed 1D storage image is emitted correctly by the very same -// SPIRV-Cross code, so the pass must not touch it - replacing working emission with our own buys -// nothing and risks everything. +// Scope, half one: a NON-arrayed 1D storage image that is only READ or WRITTEN is emitted +// correctly by the very same SPIRV-Cross code, so the pass must not touch it - replacing working +// emission with our own buys nothing and risks everything. (The atomic shape below is the one +// exception, and it is gated on an OpImageTexelPointer actually being present, which is why this +// fixture still passes through byte for byte.) TEST_F(ProgramUtilTest, Lower1DArrayImagesLeavesNonArrayed1DImagesToSpirvCross) { using namespace MG_Util::ShaderTranspiler; @@ -3489,6 +3518,94 @@ void main() { ssb.sum = imageLoad(i0, 2).r; } << "SPIRV-Cross's own 1D-as-2D emulation must still be what handles this:\n" << essl; } +// The negative control for the ATOMIC half, and the reason the non-arrayed case is in scope at +// all: SPIRV-Cross widens a 1D image coordinate in OpImageRead and OpImageWrite but not in +// OpImageTexelPointer, so the atomic comes out addressing an `uimage2D` with a scalar. Every ES +// driver answers "no matching overloaded function found" and the whole stage - with every other +// image in it - is lost. Pinning the upstream behaviour here means a future SPIRV-Cross bump that +// fixes it fails this test instead of leaving the lowering as silent dead weight. +TEST_F(ProgramUtilTest, SpirvCrossEmitsAScalarCoordinateForA1DImageAtomic) { + using namespace MG_Util::ShaderTranspiler; + + const Vector spirv = BuildSpirvForStage(k1DImageAtomicCompute, GL_COMPUTE_SHADER); + ASSERT_FALSE(spirv.empty()); + ASSERT_EQ(Count1DNonArrayedStorageImageTypes(spirv), 1u) + << "glslang no longer emits a Dim1D/non-arrayed/Sampled=2 image for uimage1D"; + + const String essl = DecompileToEssl(spirv); + ASSERT_FALSE(essl.empty()); + EXPECT_NE(essl.find("uimage2D"), String::npos) + << "SPIRV-Cross declares the 1D image as 2D on ES; that half it does do:\n" << essl; + EXPECT_NE(essl.find("imageAtomicAdd(i0, 2"), String::npos) + << "SPIRV-Cross is expected to pass the SCALAR coordinate straight through to the atomic. " + "If this no longer happens, the non-arrayed half of Lower1DArrayImagesForEssl may no " + "longer be needed:\n" + << essl; + EXPECT_EQ(essl.find("ivec2("), String::npos) + << "nothing else in this fixture builds an ivec2, so its absence is the defect:\n" << essl; +} + +// The fix: the type becomes a plain 2D image - which is what MobileGL stores a GL_TEXTURE_1D in, +// height 1 - and the coordinate becomes (u, 0), so the atomic type-checks against the declaration +// SPIRV-Cross was already emitting. +TEST_F(ProgramUtilTest, Lower1DArrayImagesWidensThe1DAtomicCoordinate) { + using namespace MG_Util::ShaderTranspiler; + + const Vector raw = BuildSpirvForStage(k1DImageAtomicCompute, GL_COMPUTE_SHADER); + ASSERT_FALSE(raw.empty()); + + Vector spirv; + ASSERT_TRUE(ShaderCompiler::SanitizeAndOptimizeBinary(raw, spirv)); + ASSERT_EQ(Count1DNonArrayedStorageImageTypes(spirv), 1u) + << "the shared chain must leave the 1D image for this pass to handle"; + + const Uint64 failuresBefore = ShaderCompiler::SpirvValidationFailureCount(); + + Vector lowered; + ASSERT_TRUE(ShaderCompiler::Lower1DArrayImagesForEssl(spirv, lowered, true)); + ASSERT_FALSE(lowered.empty()); + + EXPECT_EQ(Count1DNonArrayedStorageImageTypes(lowered), 0u) + << "no non-arrayed 1D storage image type may survive when an atomic reaches one:\n" + << DisassembleSpirv(lowered); + EXPECT_EQ(ShaderCompiler::SpirvValidationFailureCount(), failuresBefore) + << "the lowered module must stay validator-clean"; + + const String essl = DecompileToEssl(lowered); + ASSERT_FALSE(essl.empty()); + EXPECT_NE(essl.find("uimage2D"), String::npos) + << "the declaration must still be the 2D one the ES texture is:\n" << essl; + EXPECT_NE(essl.find("imageAtomicAdd(i0, ivec2(2, 0)"), String::npos) + << "the atomic must address the image with the same (u, 0) SPIRV-Cross writes for a read " + "or a write:\n" + << essl; +} + +// The declined shape for the atomic half, for the same reason as the arrayed one: after the +// rewrite the image is 2D, so imageSize() yields two components where the shader consumes one and +// there is no correct scalar to substitute. +TEST_F(ProgramUtilTest, Lower1DArrayImagesDeclinesA1DAtomicModuleThatQueriesTheImageSize) { + using namespace MG_Util::ShaderTranspiler; + + const Vector spirv = BuildSpirvForStage(R"(#version 440 core +layout (local_size_x = 1) in; +layout (r32ui) coherent uniform uimage1D i0; +layout (std430, binding = 0) buffer SSB { uint sum; } ssb; +void main() { ssb.sum = imageAtomicAdd(i0, 2, 7u) + uint(imageSize(i0)); } +)", + GL_COMPUTE_SHADER); + ASSERT_FALSE(spirv.empty()); + const auto traits = Lower1DArrayImagesPass::InspectBinary(spirv); + ASSERT_TRUE(traits.declaresImage && traits.queriesImageSize) + << "the fixture must contain the shape the pass declines"; + + Vector lowered; + ASSERT_TRUE(ShaderCompiler::Lower1DArrayImagesForEssl(spirv, lowered, true)); + EXPECT_EQ(lowered, spirv) << "a declined module must be handed back untouched, not partly rewritten"; + EXPECT_EQ(Count1DNonArrayedStorageImageTypes(lowered), 1u) + << "declining means the 1D type is still there for the driver to reject"; +} + // Scope, half two: a 1D-array SAMPLER reaches SPIRV-Cross's sampler path, which does check // `arrayed` and does move the layer into the third component. The pass is storage-image only. TEST_F(ProgramUtilTest, Lower1DArrayImagesLeavesSampledImagesAlone) { diff --git a/MobileGL/MG_Util/ShaderTranspiler/ShaderCompiler.cpp b/MobileGL/MG_Util/ShaderTranspiler/ShaderCompiler.cpp index 5f64bf34..c691f5e8 100644 --- a/MobileGL/MG_Util/ShaderTranspiler/ShaderCompiler.cpp +++ b/MobileGL/MG_Util/ShaderTranspiler/ShaderCompiler.cpp @@ -948,26 +948,27 @@ namespace MobileGL { using namespace spvtools; // Declined rather than half-translated: after the rewrite the image is a 2D - // array, so a size query on it yields three components where the shader consumes - // two. Handing back a differently-shaped size silently is worse than leaving the - // module alone and letting the driver say what it does not like - and unlike the - // access path there is no correct answer to substitute, because the ES texture + // (array) one, so a size query on it yields a component more than the shader + // consumes. Handing back a differently-shaped size silently is worse than leaving + // the module alone and letting the driver say what it does not like - and unlike + // the access path there is no correct answer to substitute, because the ES texture // genuinely has a height the GL one does not. // // MGLOG_W, latched: per shader compile, and shader packs compile lazily // mid-session. (Parked at MGLOG_I until the Log.h ordering fix made W live.) const auto traits = Lower1DArrayImagesPass::InspectBinary(inputBinary); // The overwhelmingly common answer, and the reason the inspection exists: no - // 1D-array storage image, so the module is handed back byte for byte without an - // Optimizer ever being built. Every ESSL shader in the process passes through - // here, so the cost of the case with nothing to do is the cost of this pass. + // 1D storage image this pass owns, so the module is handed back byte for byte + // without an Optimizer ever being built. Every ESSL shader in the process passes + // through here, so the cost of the case with nothing to do is the cost of this + // pass. if (!traits.declaresImage) { outputBinary = inputBinary; return true; } if (traits.queriesImageSize) { - MGLOG_W_ONCE("[spirv] Lower1DArrayImagesForEssl: the module queries the size of a 1D-array " - "storage image, which cannot be answered in the 2D-array shape ES stores it in; " + MGLOG_W_ONCE("[spirv] Lower1DArrayImagesForEssl: the module queries the size of a 1D " + "storage image, which cannot be answered in the 2D shape ES stores it in; " "leaving the module alone, and a strict ES driver will reject it"); outputBinary = inputBinary; return true; @@ -975,8 +976,8 @@ namespace MobileGL { Optimizer optimizer(SPV_ENV_VULKAN_1_1); optimizer.RegisterPass(Lower1DArrayImagesPass::CreateLower1DArrayImagesPass()); - // Mandatory, not tidying. Rewriting a 1D-array image type to the 2D-array one - // makes it structurally IDENTICAL to any real 2D-array image of the same sampled + // Mandatory, not tidying. Rewriting a 1D(-array) image type to the 2D(-array) one + // makes it structurally IDENTICAL to any real 2D(-array) image of the same sampled // type and format that the module already declared - and SPIR-V forbids duplicate // non-aggregate type declarations, so the result fails validation. That collision // is not exotic: it is the shape of this whole change's headline case, where one diff --git a/MobileGL/MG_Util/ShaderTranspiler/ShaderCompiler.h b/MobileGL/MG_Util/ShaderTranspiler/ShaderCompiler.h index ca27281c..84e84ab6 100644 --- a/MobileGL/MG_Util/ShaderTranspiler/ShaderCompiler.h +++ b/MobileGL/MG_Util/ShaderTranspiler/ShaderCompiler.h @@ -168,10 +168,13 @@ namespace MobileGL { bool enableSpirvValidation = false); // GL_TEXTURE_1D_ARRAY storage images rewritten to the 2D-array shape the texture // is actually stored in on ES, with the layer moved from the coordinate's second - // component to its third. DirectGLES transpile path only - Vulkan binds a real - // VK_IMAGE_VIEW_TYPE_1D_ARRAY and must see the module unchanged. Copies the input - // through untouched when the module declares no such image, which is every shader - // but a handful. See Lower1DArrayImagesPass for what it declines and why. + // component to its third - and, when the module performs an image ATOMIC on one, + // the non-arrayed GL_TEXTURE_1D storage image to the 2D shape with its coordinate + // widened to (u, 0), which is the one 1D shape SPIRV-Cross does not widen itself. + // DirectGLES transpile path only - Vulkan binds a real VK_IMAGE_VIEW_TYPE_1D(_ARRAY) + // and must see the module unchanged. Copies the input through untouched when the + // module declares no such image, which is every shader but a handful. See + // Lower1DArrayImagesPass for what it declines and why. static bool Lower1DArrayImagesForEssl(const Vector& inputBinary, Vector& outputBinary, bool enableSpirvValidation = false); diff --git a/MobileGL/MG_Util/ShaderTranspiler/SpirvPasses/Lower1DArrayImagesPass.cpp b/MobileGL/MG_Util/ShaderTranspiler/SpirvPasses/Lower1DArrayImagesPass.cpp index 0f444611..ac8610b9 100644 --- a/MobileGL/MG_Util/ShaderTranspiler/SpirvPasses/Lower1DArrayImagesPass.cpp +++ b/MobileGL/MG_Util/ShaderTranspiler/SpirvPasses/Lower1DArrayImagesPass.cpp @@ -49,10 +49,22 @@ namespace MobileGL { imageType->GetSingleWordInOperand(kSampledOperand) == 2u; } + // The other half of the 1D storage-image family: not arrayed. SPIRV-Cross emits + // read and write through one of these correctly, and an ATOMIC through one + // incorrectly (see the header), so this predicate only ever decides anything + // together with the atomic probe below. + bool Is1DNonArrayedStorageImageType(const Instruction* imageType) { + return imageType != nullptr && imageType->opcode() == spv::Op::OpTypeImage && + imageType->NumInOperands() > kSampledOperand && + static_cast(imageType->GetSingleWordInOperand(kDimOperand)) == spv::Dim::Dim1D && + imageType->GetSingleWordInOperand(kArrayedOperand) == 0u && + imageType->GetSingleWordInOperand(kSampledOperand) == 2u; + } + // Any Dim1D image, sampled or storage. Used only to decide whether the Image1D // capability is still needed - deliberately wider than the rewrite's own - // predicate, so a module that also holds a non-arrayed 1D image (which this pass - // leaves to SPIRV-Cross) keeps the capability it still requires. + // predicate, so a module that also holds a 1D image this pass left alone keeps the + // capability it still requires. bool IsDim1DImageType(const Instruction* imageType) { return imageType != nullptr && imageType->opcode() == spv::Op::OpTypeImage && imageType->NumInOperands() > kSampledOperand && @@ -110,6 +122,60 @@ namespace MobileGL { return opcode == spv::Op::OpImageQuerySize || opcode == spv::Op::OpImageQuerySizeLod || opcode == spv::Op::OpImageQueryLevels || opcode == spv::Op::OpImageQuerySamples; } + + // Which 1D storage images this module is to be rewritten for. Arrayed ones always; + // non-arrayed ones only when an atomic reaches one, because that is the only shape + // SPIRV-Cross gets wrong for them and taking over a path it gets right would be a + // regression looking for somewhere to happen. + struct LoweringScope { + bool arrayed = false; + bool nonArrayed = false; + + bool Any() const { return arrayed || nonArrayed; } + bool Covers(const Instruction* imageType) const { + return (arrayed && Is1DArrayStorageImageType(imageType)) || + (nonArrayed && Is1DNonArrayedStorageImageType(imageType)); + } + }; + + // OpImageTexelPointer is the operand path of every imageAtomic*; nothing else in a + // GLSL-derived module produces one. + bool PerformsAtomicOnNonArrayed1DImage(IRContext* context) { + for (auto& function : *context->module()) { + for (auto& block : function) { + for (auto& instruction : block) { + if (instruction.opcode() != spv::Op::OpImageTexelPointer || + instruction.NumInOperands() < 1) { + continue; + } + if (Is1DNonArrayedStorageImageType( + ResolveImageType(context, instruction.GetSingleWordInOperand(0)))) { + return true; + } + } + } + } + return false; + } + + // One walk of the type table, then - and only when the module declares a + // non-arrayed 1D storage image at all - one walk of the code. Every other shader + // pays the type walk and nothing else. + LoweringScope ResolveLoweringScope(IRContext* context) { + LoweringScope scope; + bool hasNonArrayed = false; + for (const Instruction& type : context->module()->types_values()) { + if (Is1DArrayStorageImageType(&type)) { + scope.arrayed = true; + } else if (Is1DNonArrayedStorageImageType(&type)) { + hasNonArrayed = true; + } + } + if (hasNonArrayed) { + scope.nonArrayed = PerformsAtomicOnNonArrayed1DImage(context); + } + return scope; + } } // namespace Lower1DArrayImagesPass::ModuleTraits Lower1DArrayImagesPass::InspectBinary(const Vector& binary) { @@ -126,15 +192,11 @@ namespace MobileGL { // The type table settles it for the cheap half, and it is the half almost every // shader takes: no such type declared, nothing to inspect further. - for (const Instruction& type : context->module()->types_values()) { - if (Is1DArrayStorageImageType(&type)) { - traits.declaresImage = true; - break; - } - } - if (!traits.declaresImage) { + const LoweringScope scope = ResolveLoweringScope(context.get()); + if (!scope.Any()) { return traits; } + traits.declaresImage = true; for (auto& function : *context->module()) { for (auto& block : function) { @@ -142,7 +204,7 @@ namespace MobileGL { if (!QueriesImageSize(instruction.opcode()) || instruction.NumInOperands() < 1) { continue; } - if (Is1DArrayStorageImageType( + if (scope.Covers( ResolveImageType(context.get(), instruction.GetSingleWordInOperand(0)))) { traits.queriesImageSize = true; return traits; @@ -160,27 +222,21 @@ namespace MobileGL { // Nothing to do unless the module actually declares one. Every other shader pays // one walk of the type table and is handed back unchanged. - bool hasType = false; - for (const Instruction& type : irContext->types_values()) { - if (Is1DArrayStorageImageType(&type)) { - hasType = true; - break; - } - } - if (!hasType) { + const LoweringScope scope = ResolveLoweringScope(irContext); + if (!scope.Any()) { return Status::SuccessWithoutChange; } // The same refusal the caller makes, restated here so the pass is safe wherever // it is registered. Rewriting the type while leaving an OpImageQuerySize on it // produces a query whose result type has one component too few - an invalid - // module - and there is no correct two-component size to substitute, because the - // ES texture genuinely has a height the GL one does not. + // module - and there is no correct narrower size to substitute, because the ES + // texture genuinely has a height the GL one does not. for (auto& function : *irContext->module()) { for (auto& block : function) { for (auto& instruction : block) { if (QueriesImageSize(instruction.opcode()) && instruction.NumInOperands() >= 1 && - Is1DArrayStorageImageType( + scope.Covers( ResolveImageType(irContext, instruction.GetSingleWordInOperand(0)))) { return Status::SuccessWithoutChange; } @@ -188,9 +244,12 @@ namespace MobileGL { } } - // (u, layer) -> (u, 0, layer). The height the ES 2D array carries is 1, so Y is - // always 0 and the layer has to move from the second component to the third; a - // plain widening that appended the 0 would read layer 0 of every access instead. + // Arrayed: (u, layer) -> (u, 0, layer). The height the ES 2D array carries is 1, + // so Y is always 0 and the layer has to move from the second component to the + // third; a plain widening that appended the 0 would read layer 0 of every access + // instead. Non-arrayed: u -> (u, 0), which is exactly what SPIRV-Cross itself + // writes for the operations it does widen - reproduced here so read, write and + // atomic all come out of one place. for (auto& function : *irContext->module()) { for (auto& block : function) { for (auto& instruction : block) { @@ -199,38 +258,44 @@ namespace MobileGL { instruction.NumInOperands() <= coordinateOperand) { continue; } - if (!Is1DArrayStorageImageType( - ResolveImageType(irContext, instruction.GetSingleWordInOperand(0)))) { + const Instruction* imageType = + ResolveImageType(irContext, instruction.GetSingleWordInOperand(0)); + if (!scope.Covers(imageType)) { continue; } + const bool arrayed = Is1DArrayStorageImageType(imageType); const uint32_t coordinateId = instruction.GetSingleWordInOperand(coordinateOperand); // Built from the COORDINATE's own component type rather than a - // hardcoded signed int. GLSL only ever spells these ivec2, but SPIR-V - // permits an unsigned coordinate, and extracting a uint component - // into an int result is an invalid module rather than a wrong answer - - // the kind of defect that reaches a driver as "compiles here, not - // there". + // hardcoded signed int. GLSL only ever spells these int/ivec2, but + // SPIR-V permits an unsigned coordinate, and extracting a uint + // component into an int result is an invalid module rather than a + // wrong answer - the kind of defect that reaches a driver as "compiles + // here, not there". Instruction* coordinateDef = irContext->get_def_use_mgr()->GetDef(coordinateId); if (coordinateDef == nullptr) return Status::Failure; const auto* coordinateType = typeMgr->GetType(coordinateDef->type_id()); - const auto* coordinateVector = coordinateType != nullptr ? coordinateType->AsVector() - : nullptr; - if (coordinateVector == nullptr || coordinateVector->element_count() != 2) { + if (coordinateType == nullptr) return Status::Failure; + // Arrayed coordinates are the two-component (u, layer); non-arrayed + // ones are the bare scalar u. Anything else is a shape this pass does + // not translate, and declining leaves the module byte for byte. + const auto* coordinateVector = arrayed ? coordinateType->AsVector() : nullptr; + if (arrayed && (coordinateVector == nullptr || coordinateVector->element_count() != 2)) { return Status::Failure; } - const auto* component = coordinateVector->element_type(); + const auto* component = + arrayed ? coordinateVector->element_type() : coordinateType; const auto* componentInteger = component != nullptr ? component->AsInteger() : nullptr; if (componentInteger == nullptr) return Status::Failure; - spvtools::opt::analysis::Vector widenedVector(component, 3); - const uint32_t int3TypeId = typeMgr->GetTypeInstruction(&widenedVector); + spvtools::opt::analysis::Vector widenedVector(component, arrayed ? 3 : 2); + const uint32_t widenedTypeId = typeMgr->GetTypeInstruction(&widenedVector); const uint32_t intTypeId = typeMgr->GetTypeInstruction(component); const uint32_t zeroId = componentInteger->IsSigned() ? constantMgr->GetSIntConstId(0) : constantMgr->GetUIntConstId(0); - if (int3TypeId == 0 || intTypeId == 0 || zeroId == 0) { + if (widenedTypeId == 0 || intTypeId == 0 || zeroId == 0) { return Status::Failure; } @@ -238,15 +303,20 @@ namespace MobileGL { irContext, &instruction, IRContext::kAnalysisDefUse | IRContext::kAnalysisInstrToBlockMapping); - Instruction* u = - builder.AddCompositeExtract(intTypeId, coordinateId, {0}); - Instruction* layer = - builder.AddCompositeExtract(intTypeId, coordinateId, {1}); - if (u == nullptr || layer == nullptr) { - return Status::Failure; + Instruction* widened = nullptr; + if (arrayed) { + Instruction* u = + builder.AddCompositeExtract(intTypeId, coordinateId, {0}); + Instruction* layer = + builder.AddCompositeExtract(intTypeId, coordinateId, {1}); + if (u == nullptr || layer == nullptr) { + return Status::Failure; + } + widened = builder.AddCompositeConstruct( + widenedTypeId, {u->result_id(), zeroId, layer->result_id()}); + } else { + widened = builder.AddCompositeConstruct(widenedTypeId, {coordinateId, zeroId}); } - Instruction* widened = builder.AddCompositeConstruct( - int3TypeId, {u->result_id(), zeroId, layer->result_id()}); if (widened == nullptr) { return Status::Failure; } @@ -256,21 +326,22 @@ namespace MobileGL { } } - // Only now, with no access still spelling the 1D-array coordinate, does the type - // become the 2D array one. Arrayed stays 1: this is a 2D ARRAY image, which is - // what the texture was stored as. + // Only now, with no access still spelling a 1D coordinate, does the type become + // the 2D one. Arrayed is left exactly as it was - a 1D array becomes a 2D ARRAY + // image, which is what the texture was stored as, and a non-arrayed 1D becomes the + // plain 2D image MobileGL stores a GL_TEXTURE_1D in (height 1). for (Instruction& type : irContext->types_values()) { - if (Is1DArrayStorageImageType(&type)) { + if (scope.Covers(&type)) { type.SetInOperand(kDimOperand, {static_cast(spv::Dim::Dim2D)}); } } // Image1D describes the types just rewritten - but only drop it if no 1D image - // type is left at all. A module may hold a non-arrayed 1D storage image, which - // this pass deliberately leaves to SPIRV-Cross, and that one still needs the - // capability. Shader is always declared by any module reaching here, so restating - // it keeps the instruction valid without leaving a capability a consumer could - // key off. + // type is left at all. A module may hold a 1D image this pass left alone (a + // SAMPLED one always, and a non-arrayed storage one whenever no atomic reaches + // it), and that one still needs the capability. Shader is always declared by any + // module reaching here, so restating it keeps the instruction valid without + // leaving a capability a consumer could key off. bool anyDim1DLeft = false; for (const Instruction& type : irContext->types_values()) { if (IsDim1DImageType(&type)) { diff --git a/MobileGL/MG_Util/ShaderTranspiler/SpirvPasses/Lower1DArrayImagesPass.h b/MobileGL/MG_Util/ShaderTranspiler/SpirvPasses/Lower1DArrayImagesPass.h index 10b3f5c6..72b27a2f 100644 --- a/MobileGL/MG_Util/ShaderTranspiler/SpirvPasses/Lower1DArrayImagesPass.h +++ b/MobileGL/MG_Util/ShaderTranspiler/SpirvPasses/Lower1DArrayImagesPass.h @@ -46,13 +46,30 @@ namespace MobileGL { // through it has its coordinate widened from (u, layer) to (u, 0, layer). SPIRV-Cross // is then looking at an ordinary 2D array image and its 1D path never fires. // + // The NON-arrayed 1D storage image is handled too, but only in one shape. SPIRV-Cross + // applies the widening above in OpImageRead (spirv_glsl.cpp) and in OpImageWrite - and + // NOT in OpImageTexelPointer, which is the operand path every imageAtomic* goes + // through. So `imageAtomicAdd(g_image_1d, coord.x, 2)` comes out with a SCALAR + // coordinate against a variable it declared `iimage2D`, and the ES compiler answers + // "'imageAtomicAdd' : no matching overloaded function found" - losing the whole stage + // and with it every other image in it, which is how + // KHR-GL4x.shader_image_load_store.basic-allTargets-atomic lost a seven-image fragment + // shader over one of them. + // + // That case is lowered here for the same reason as the arrayed one: the type becomes + // Dim2D and every coordinate is widened from u to (u, 0) in the module, so SPIRV-Cross + // has no 1D image left to emulate and read, write and atomic are all spelled by one + // piece of code. It is gated on the module ACTUALLY performing an image atomic on such + // an image, so a shader that only loads and stores through a 1D image keeps taking + // SPIRV-Cross's own (correct) emission byte for byte and this pass cannot regress it. + // // Deliberately narrow, on three axes: // // * STORAGE images only (Sampled == 2). Sampled images reach SPIRV-Cross's sampler // path, which is correct today; rewriting them would replace working emission // with our own for no reason. - // * ARRAYED only. A non-arrayed 1D storage image is emitted correctly by the same - // SPIRV-Cross code, and is left to it. + // * ARRAYED always; NON-arrayed only when the module holds an OpImageTexelPointer + // into one, i.e. only when SPIRV-Cross's own emission is already broken for it. // * ESSL only. Vulkan has VK_IMAGE_VIEW_TYPE_1D_ARRAY natively and Magma binds it // directly, so the module must reach that backend unchanged. // @@ -81,13 +98,15 @@ namespace MobileGL { // cost one module parse and no optimizer run at all - not one parse to ask about // size queries and a second inside an Optimizer that then early-outs. struct ModuleTraits { - // The module declares a 1D-array storage image, i.e. there is anything to do. + // The module declares an image this pass would rewrite - a 1D-array storage + // image, or a non-arrayed 1D storage image the module performs an atomic on - + // i.e. there is anything to do. bool declaresImage = false; // ...and queries its size, which is the shape this pass refuses to translate: - // afterwards the image is a 2D array, so the query yields three components - // where the shader consumes two, and there is no correct two-component answer - // to substitute. The caller leaves such a module alone rather than half - // rewriting it. + // afterwards the image is a 2D (array) one, so the query yields a component + // more than the shader consumes, and there is no correct narrower answer to + // substitute. The caller leaves such a module alone rather than half rewriting + // it. bool queriesImageSize = false; }; static ModuleTraits InspectBinary(const Vector& binary);