[Fix, Test] (ShaderTranspiler, DirectGLES): widen a non-arrayed 1D storage image's atomic coordinate

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<unsigned int> arrayImageSpirv;
                 if (MG_Util::ShaderTranspiler::ShaderCompiler::Lower1DArrayImagesForEssl(*effectiveSpirv,
                                                                                           arrayImageSpirv, enableSpirvValidation) &&

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<Uint32>& 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<Uint32> 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<Uint32> raw = BuildSpirvForStage(k1DImageAtomicCompute, GL_COMPUTE_SHADER);
+    ASSERT_FALSE(raw.empty());
+
+    Vector<Uint32> 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<Uint32> 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<Uint32> 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<Uint32> 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) {

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

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<Uint32>& inputBinary,
                                                       Vector<uint32_t>& outputBinary,
                                                       bool enableSpirvValidation = false);

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<spv::Dim>(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<Uint32>& 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<uint32_t>(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)) {

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<Uint32>& binary);