[Fix, Test] (ShaderTranspiler): carry r11f_g11f_b10f storage images in rgba16f instead of losing the stage

MobileGL/MG_Test/ShaderTranspiler/WidenImageFormatsTest.cpp

@@ -14,10 +14,10 @@
 // and every draw with the program silently renders nothing while GL_LINK_STATUS still says TRUE.
 //
 // What has to hold is the emulation's exactness, in three parts at once: the DECLARED format must
-// become the core carrier of the same per-channel width, every imageStore through it must have its
+// become a core carrier that loses nothing, every imageStore through it must have its surplus
-// surplus components replaced by GL's own (0.., 1) so the carrier's extra channels never hold
+// components replaced by GL's own (0.., 1) so the carrier's extra channels never hold anything GL
-// anything GL has not defined, and every imageLoad must come back masked the same way. A module
+// has not defined, and every imageLoad must come back masked the same way. A module that declares
-// that declares only core formats - or one of the nine formats with no exact carrier - must come
+// only core formats - or one of the eight formats with no lossless carrier at all - must come
 // out untouched, because widening those would be an approximation rather than an emulation. Real
 // GLSL through the same glslang path the backends use, for the same reason
 // ClampMultisampleFetchTest.cpp does it: what matters is what glslang actually emits.
@@ -229,9 +229,23 @@ void main() {
 }
 )";
 
-    // rg16 is one of the NINE with no core carrier of the same per-channel width. Widening it
+    // r11f_g11f_b10f: THREE float channels in a packed 32-bit word, and the only format the four
-    // would change the quantisation an application sees, so it must be left alone and keep the
+    // CTS allFormats/allTargets walkers still aborted on after the channel widening landed - it
-    // honest "no GLSL ES spelling" diagnostic instead.
+    // has no core carrier of the same per-channel width, so it took rgba16f, whose 5-bit exponent
+    // and longer mantissa represent every 11f and 10f value exactly.
+    const char* const kR11fG11fB10fLoadStore = R"(#version 430 core
+layout(r11f_g11f_b10f, binding = 0) uniform image2D img;
+out vec4 fragColor;
+void main() {
+    vec4 texel = imageLoad(img, ivec2(gl_FragCoord.xy));
+    imageStore(img, ivec2(gl_FragCoord.xy), vec4(1.0, 2.0, 3.0, 4.0));
+    fragColor = texel;
+}
+)";
+
+    // rg16 is one of the EIGHT with no core carrier at all - core ESSL has no 16-bit normalized
+    // format, so every candidate loses range or changes the component type the texture presents.
+    // It must be left alone and keep the honest "no GLSL ES spelling" diagnostic instead.
     const char* const kRg16LoadStore = R"(#version 430 core
 layout(rg16, binding = 0) uniform image2D img;
 out vec4 fragColor;
@@ -247,7 +261,7 @@ void main() {
 // the shader rewrite, the ES texture storage and the glBindImageTexture argument. If it drifts
 // the three stop agreeing, and a narrow texture read through a wide image goes out of bounds
 // silently on every driver tested.
-TEST(WidenImageFormats, SeventeenNonCoreFormatsHaveAnExactSameWidthCarrier) {
+TEST(WidenImageFormats, EighteenNonCoreFormatsHaveALosslessCoreCarrier) {
     struct Case {
         Uint requested;
         Uint carrier;
@@ -272,6 +286,10 @@ TEST(WidenImageFormats, SeventeenNonCoreFormatsHaveAnExactSameWidthCarrier) {
         {0x8234, 0x8D76, 1, "GL_R16UI -> GL_RGBA16UI"},
         {0x8238, 0x8D7C, 2, "GL_RG8UI -> GL_RGBA8UI"},
         {0x8232, 0x8D7C, 1, "GL_R8UI -> GL_RGBA8UI"},
+        // The one entry that is a re-encoding rather than a channel widening: 11f is e5m6 and 10f
+        // is e5m5 against a half's s1e5m10, so the carrier is still lossless - and three channels,
+        // so the mask has to pin only alpha.
+        {0x8C3A, 0x881A, 3, "GL_R11F_G11F_B10F -> GL_RGBA16F"},
     };
     for (const Case& testCase : cases) {
         EXPECT_EQ(ShaderCompiler::WidenedCoreEsslImageFormat(testCase.requested), testCase.carrier)
@@ -287,7 +305,7 @@ TEST(WidenImageFormats, SeventeenNonCoreFormatsHaveAnExactSameWidthCarrier) {
     }
 }
 
-TEST(WidenImageFormats, CoreFormatsAndTheNineWithoutAnExactCarrierAreRefused) {
+TEST(WidenImageFormats, CoreFormatsAndTheEightWithoutALosslessCarrierAreRefused) {
     // The thirteen GLSL ES already has: nothing to carry.
     for (const Uint coreFormat : {0x8814u /*RGBA32F*/, 0x881Au /*RGBA16F*/, 0x822Eu /*R32F*/,
                                   0x8058u /*RGBA8*/, 0x8F97u /*RGBA8_SNORM*/, 0x8D82u /*RGBA32I*/,
@@ -297,10 +315,12 @@ TEST(WidenImageFormats, CoreFormatsAndTheNineWithoutAnExactCarrierAreRefused) {
         EXPECT_EQ(ShaderCompiler::WidenedCoreEsslImageFormat(coreFormat), 0u)
             << "core format 0x" << std::hex << coreFormat;
     }
-    // The nine with no core format of the same per-channel width. Carrying these would be an
+    // The eight with no LOSSLESS core carrier: core ESSL has no 16-bit normalized format and no
-    // approximation - a different quantisation, or a different numeric domain for anything that
+    // 10-bit one, so every candidate for these either loses range or changes the component type
-    // samples the same texture - so they are deliberately left to the honest diagnostic.
+    // the texture presents to anything that samples it. Deliberately left to the honest
-    for (const Uint hardFormat : {0x8C3Au /*R11F_G11F_B10F*/, 0x8059u /*RGB10_A2*/,
+    // diagnostic. r11f_g11f_b10f is NOT among them - rgba16f holds every value it can, so it is
+    // carried above.
+    for (const Uint hardFormat : {0x8059u /*RGB10_A2*/,
                                   0x906Fu /*RGB10_A2UI*/, 0x805Bu /*RGBA16*/, 0x822Cu /*RG16*/,
                                   0x822Au /*R16*/, 0x8F9Bu /*RGBA16_SNORM*/, 0x8F99u /*RG16_SNORM*/,
                                   0x8F98u /*R16_SNORM*/}) {
@@ -357,6 +377,68 @@ TEST(WidenImageFormats, TwoChannelFloatImageBecomesRgba32fWithBothAccessesMasked
         << "the mask must be a separate value, or it would feed itself";
 }
 
+// The three-channel case, which no format exercised before r11f_g11f_b10f was carried: only ALPHA
+// is surplus, so the mask must take r, g and b from the texel and nothing but the fourth component
+// from the (0, 0, 0, 1) constant. A mask that zeroed blue here - the shape a two-channel format
+// wants - would silently drop the third channel of every store.
+TEST(WidenImageFormats, ThreeChannelPackedFloatImageBecomesRgba16fWithOnlyAlphaPinned) {
+    const Vector<Uint32> spirv = CompileFragment(kR11fG11fB10fLoadStore);
+    ASSERT_FALSE(spirv.empty());
+    ASSERT_TRUE(ShaderCompiler::DeclaresWidenableImageFormat(spirv));
+
+    const auto beforeTypes = CollectStorageImageTypes(spirv);
+    ASSERT_EQ(beforeTypes.size(), 1u);
+    EXPECT_EQ(beforeTypes.front().format, static_cast<Uint32>(spv::ImageFormat::R11fG11fB10f));
+
+    Vector<Uint32> widened;
+    ASSERT_TRUE(ShaderCompiler::WidenImageFormatsForEssl(spirv, widened, /*onlyFormatsSpirvCrossRefusesToPrint=*/false,
+                                                         /*enableSpirvValidation=*/true));
+    ASSERT_FALSE(widened.empty());
+    EXPECT_TRUE(Validates(widened));
+    EXPECT_FALSE(ShaderCompiler::DeclaresWidenableImageFormat(widened));
+
+    const auto afterTypes = CollectStorageImageTypes(widened);
+    ASSERT_EQ(afterTypes.size(), 1u);
+    EXPECT_EQ(afterTypes.front().format, static_cast<Uint32>(spv::ImageFormat::Rgba16f));
+
+    const auto shuffles = CollectVectorShuffles(widened);
+
+    const auto texelIds = CollectImageWriteTexelIds(widened);
+    ASSERT_EQ(texelIds.size(), 1u);
+    const VectorShuffle* storeMask = FindShuffleWithResult(shuffles, texelIds.front());
+    ASSERT_NE(storeMask, nullptr) << "the imageStore texel is not a masked value";
+    EXPECT_TRUE(HasComponents(*storeMask, {0u, 1u, 2u, 7u}))
+        << "expected (r, g, b, 1) - components 0, 1 and 2 of the texel, then 3 of (0,0,0,1)";
+
+    const auto readIds = CollectImageReadResultIds(widened);
+    ASSERT_EQ(readIds.size(), 1u);
+    const VectorShuffle* loadMask = FindShuffleOver(shuffles, readIds.front());
+    ASSERT_NE(loadMask, nullptr) << "the imageLoad result is consumed unmasked";
+    EXPECT_TRUE(HasComponents(*loadMask, {0u, 1u, 2u, 7u}));
+}
+
+// ...and the same module through the emitter, which is where the failure actually showed: ESSL has
+// no `r11f_g11f_b10f` token, SPIRV-Cross throws for it, and the throw took every image uniform
+// declared in the same stage with it.
+TEST(WidenImageFormats, PackedFloatImageOnlyReachesEsslThroughTheCarrier) {
+    const Vector<Uint32> spirv = CompileFragment(kR11fG11fB10fLoadStore);
+    ASSERT_FALSE(spirv.empty());
+
+    const EsslAttempt before = EmitEssl(spirv);
+    EXPECT_FALSE(before.succeeded)
+        << "SPIRV-Cross printed r11f_g11f_b10f for an ES target; the widening's premise has "
+           "changed:\n"
+        << before.text;
+
+    Vector<Uint32> widened;
+    ASSERT_TRUE(ShaderCompiler::WidenImageFormatsForEssl(spirv, widened, /*onlyFormatsSpirvCrossRefusesToPrint=*/false,
+                                                         /*enableSpirvValidation=*/true));
+    const EsslAttempt after = EmitEssl(widened);
+    ASSERT_TRUE(after.succeeded) << after.error;
+    EXPECT_NE(after.text.find("rgba16f"), String::npos) << after.text;
+    EXPECT_EQ(after.text.find("r11f_g11f_b10f"), String::npos) << after.text;
+}
+
 TEST(WidenImageFormats, SingleChannelUnsignedImageBecomesRgba8uiWithBothAccessesMasked) {
     const Vector<Uint32> spirv = CompileFragment(kR8uiLoadStore);
     ASSERT_FALSE(spirv.empty());

MobileGL/MG_Util/ShaderTranspiler/SpirvPasses/WidenImageFormatsPass.cpp

@@ -50,16 +50,40 @@ namespace MobileGL {
                 constexpr uint32_t kImageAccessImageOperand = 0;
                 constexpr uint32_t kImageWriteTexelOperand = 2;
 
-                // The exact carrier of a non-core image format: the core GLSL ES format with the
+                // The carrier of a non-core image format: a core GLSL ES format that represents
-                // SAME component type and the SAME per-channel width, differing only in channel
+                // every value the original can hold, WITHOUT LOSS. `channels` is what the original
-                // count. `channels` is what the original format really has, which is what every
+                // format really has, which is what every access through the carrier is masked back
-                // access through the carrier is masked back to.
+                // to.
                 //
-                // Only formats that widen EXACTLY appear here. r11f_g11f_b10f, rgb10_a2,
+                // Almost every entry is a pure CHANNEL widening - same component type, same
-                // rgb10_a2ui, rgba16, rg16, r16, rgba16_snorm, rg16_snorm and r16_snorm have no
+                // per-channel width, more channels (rg32f -> rgba32f) - and for those the carrier
-                // same-width core carrier - every candidate is either lossy or changes the numeric
+                // is bit-exact: the storage holds the identical encoding, only wider.
-                // domain a sampler would read - and are deliberately absent, so they keep the
+                //
-                // honest "no GLSL ES spelling" diagnostic rather than a silent approximation.
+                // r11f_g11f_b10f is the one entry that is not. It has no same-width core carrier,
+                // so it takes rgba16f, and the two encodings differ. What matters is that the
+                // carrier is still LOSSLESS: an 11-bit float is e5m6 and a 10-bit float is e5m5,
+                // while a half is s1e5m10 - the SAME 5-bit exponent with a strictly longer
+                // mantissa - so every value the packed format can represent has an exact half.
+                // Nothing an application stores is rounded away.
+                //
+                // What DOES change is the reverse direction: the carrier can hold values the
+                // packed format could not - negatives (11f and 10f are unsigned), and mantissa
+                // bits finer than the 6 and 5 the format quantises to - so a value written through
+                // the image and then SAMPLED comes back on half's grid rather than the packed
+                // format's. That is a strictly finer grid, never a lossy one, and it is measured
+                // against the alternative, which is not a more faithful quantisation but no
+                // program at all: `layout(r11f_g11f_b10f)` has no ESSL spelling, SPIRV-Cross
+                // throws for it, and the stage - with every other image uniform declared beside it
+                // - is lost (KHR-GL43.shader_image_load_store.basic-allFormats-*, which fail on
+                // this format alone, and multiple-uniforms, where one such declaration killed a
+                // program holding eight images).
+                //
+                // The remaining eight - rgb10_a2, rgb10_a2ui, rgba16, rg16, r16, rgba16_snorm,
+                // rg16_snorm and r16_snorm - stay absent, and for a stronger reason than
+                // quantisation: core ESSL has no 16-bit normalized format at all and no 10-bit
+                // one, so every candidate carrier for them either loses range or changes the
+                // component TYPE the texture presents. They keep the honest "no GLSL ES spelling"
+                // diagnostic rather than a silent approximation.
                 struct ImageFormatWidening {
                     spv::ImageFormat Carrier = spv::ImageFormat::Unknown;
                     uint32_t Channels = 0;
@@ -73,6 +97,9 @@ namespace MobileGL {
                     case spv::ImageFormat::Rg32f: return {spv::ImageFormat::Rgba32f, 2};
                     case spv::ImageFormat::Rg16f: return {spv::ImageFormat::Rgba16f, 2};
                     case spv::ImageFormat::R16f: return {spv::ImageFormat::Rgba16f, 1};
+                    // Not a channel widening but a lossless re-encoding - see above. Three
+                    // channels, so the fourth reads as the 1 GL defines for a format without one.
+                    case spv::ImageFormat::R11fG11fB10f: return {spv::ImageFormat::Rgba16f, 3};
                     // Unsigned normalized.
                     case spv::ImageFormat::Rg8: return {spv::ImageFormat::Rgba8, 2};
                     case spv::ImageFormat::R8: return {spv::ImageFormat::Rgba8, 1};

MobileGL/MG_Util/ShaderTranspiler/SpirvPasses/WidenImageFormatsPass.h

@@ -54,12 +54,23 @@ namespace MobileGL {
             // alone survives storage this shader never wrote (glTexStorage with no upload, whose
             // surplus channels are undefined).
             //
-            // The other NINE (r11f_g11f_b10f, rgb10_a2, rgb10_a2ui, rgba16, rg16, r16,
+            // r11f_g11f_b10f has no same-width core carrier either, and takes rgba16f anyway,
-            // rgba16_snorm, rg16_snorm, r16_snorm) have NO same-width core carrier and are
+            // because that carrier is still LOSSLESS: 11f is e5m6 and 10f is e5m5 against a half's
-            // deliberately NOT widened here: every carrier for them is either lossy or changes the
+            // s1e5m10 - the SAME 5-bit exponent with a strictly longer mantissa - so every value
-            // numeric domain of the texture a `sampler2D` would read from it. They keep the honest
+            // the packed format can hold has an exact half. Only the reverse direction differs
-            // "no GLSL ES spelling" diagnostic instead of silently changing an application's
+            // (the carrier also holds negatives, which 11f and 10f cannot sign, and mantissa bits
-            // quantisation behaviour.
+            // finer than the 6 and 5 they quantise to, so a value written through the image and
+            // then SAMPLED lands on half's grid rather than the packed format's). That is measured
+            // against the alternative, which is not a truer quantisation but no program at all:
+            // the SPIRV-Cross throw takes the whole stage, every image uniform declared beside it
+            // included.
+            //
+            // The other EIGHT (rgb10_a2, rgb10_a2ui, rgba16, rg16, r16, rgba16_snorm, rg16_snorm,
+            // r16_snorm) are deliberately NOT widened here: core ESSL has no 16-bit normalized
+            // format at all and no 10-bit one, so every carrier for them either loses range or
+            // changes the component TYPE the texture a `sampler2D` would read presents. They keep
+            // the honest "no GLSL ES spelling" diagnostic instead of silently changing an
+            // application's numeric domain.
             //
             // MUST MOVE WITH THE OTHER TWO LAYERS. The widening is not a shader-local rewrite: the
             // ES texture behind the image has to be allocated in the carrier format too, and