[Fix, Test] (ShaderTranspiler): rewrite float-equals-zero exactly instead of within a 1e-4 epsilon

MobileGL/MG_Test/ShaderTranspiler/DemoteFloat64Test.cpp

@@ -8,6 +8,7 @@
 
 #include <gtest/gtest.h>
 
+#include <sstream>
 #include <string>
 #include <vector>
 
@@ -484,52 +485,165 @@ TEST_F(DemoteFloat64Test, RejectsGarbageInput) {
     EXPECT_FALSE(ShaderCompiler::DemoteFloat64ToFloat32(notSpirv, output, true));
 }
 
-// EliminateFloatEqualsZeroPass turns a comparison against 0.0 into an epsilon test, a
+// EliminateFloatEqualsZeroPass re-spells a comparison against 0.0 through GLSL.std.450 FAbs, so
-// workaround for drivers whose exact float compare misbehaves. Deciding WHICH constants are
+// that no float-equality instruction reaches a driver that gets one wrong. Deciding WHICH
-// zero used to read every float constant as though it were 32 bits wide, and on a 64-bit
+// constants are zero used to read every float constant as though it were 32 bits wide, and on a
-// constant that reads the LOW half of the mantissa - which is zero for 1.0lf, 2.0lf, 0.5lf and
+// 64-bit constant that reads the LOW half of the mantissa - which is zero for 1.0lf, 2.0lf, 0.5lf
-// every other round double a shader is likely to spell. Each of those was mistaken for 0.0, so
+// and every other round double a shader is likely to spell. Each of those was mistaken for 0.0, so
-// a comparison against 1.0lf became an epsilon test against ZERO, and came out true for a
+// a comparison against 1.0lf became a test against ZERO, and came out true for a uniform holding
-// uniform holding exactly 1.0. That is the whole of KHR-GL43.compute_shader.fp64-case2.
+// exactly 1.0. That is the whole of KHR-GL43.compute_shader.fp64-case2.
+//
+// The replacement itself used to be an epsilon ball, `abs(x) < 1e-4`, which called any legitimately
+// small value zero: KHR-GL3x.buffer_objects.triangles computes a specular term of ~6e-5 at a large
+// render target and rendered black. It is exact now - `abs(x) <= 0.0` / `abs(x) > 0.0` against the
+// module's own zero constant - and the tests below pin both halves of that: only a genuine 0.0 is
+// matched, and what the compare tests against is the constant the source itself spelled.
 //
 // Asserted on the optimized module rather than through a driver, because that is where the
-// rewrite happens and its fingerprint there is unambiguous: the epsilon form introduces a
+// rewrite happens and its fingerprint there is unambiguous: the rewrite introduces a
 // GLSL.std.450 FAbs, and nothing else in these shaders would.
 namespace {
-    Bool RewritesToAnEpsilonTest(const String& source) {
+    String OptimizedDisassembly(const String& source) {
         const Vector<Uint32> input = CompileToSpirv(GL_COMPUTE_SHADER, source);
         EXPECT_FALSE(input.empty());
-        if (input.empty()) return false;
+        if (input.empty()) return {};
         Vector<Uint32> output;
         EXPECT_TRUE(ShaderCompiler::SanitizeAndOptimizeBinary(input, output, true, true));
-        return Disassemble(output).find("FAbs") != String::npos;
+        return Disassemble(output);
     }
 
-    String CompareAgainst(const String& type, const String& literal) {
+    Bool RewritesToAnAbsoluteValueTest(const String& source) {
+        return OptimizedDisassembly(source).find("FAbs") != String::npos;
+    }
+
+    String CompareAgainstUsing(const String& type, const String& op, const String& literal) {
         return "#version 430 core\n"
                "layout(local_size_x = 1) in;\n"
                "buffer Result { int g_result; };\n"
                "uniform " + type + " g_0;\n"
                "void main() {\n"
                "  g_result = 0;\n"
-               "  if (g_0 != " + literal + ") g_result = 1;\n"
+               "  if (g_0 " + op + " " + literal + ") g_result = 1;\n"
                "}\n";
     }
+
+    String CompareAgainst(const String& type, const String& literal) {
+        return CompareAgainstUsing(type, "!=", literal);
+    }
+
+    // Every instruction of a disassembly, split into whitespace-separated tokens, so an operand can
+    // be identified by position instead of by a substring another opcode might also contain -
+    // `OpFOrdLessThan` is a prefix of `OpFOrdLessThanEqual`, and those two are the whole difference
+    // between the epsilon rewrite and the exact one.
+    Vector<Vector<String>> TokenizedInstructions(const String& disassembly) {
+        Vector<Vector<String>> instructions;
+        StringStream lines(disassembly);
+        String line;
+        while (std::getline(lines, line)) {
+            Vector<String> tokens;
+            StringStream words(line);
+            String word;
+            while (words >> word) tokens.push_back(word);
+            instructions.push_back(tokens);
+        }
+        return instructions;
+    }
+
+    // The compare the rewrite leaves behind, e.g. `%22 = OpFOrdLessThanEqual %bool %21 %float_0`,
+    // or an empty vector if the module has none. These four opcodes are the only ones the pass
+    // emits and nothing else in these shaders produces one.
+    Vector<String> FindRewrittenCompare(const String& disassembly) {
+        for (const Vector<String>& tokens : TokenizedInstructions(disassembly)) {
+            if (tokens.size() < 6 || tokens[1] != "=") continue;
+            if (tokens[2] == "OpFOrdLessThanEqual" || tokens[2] == "OpFUnordLessThanEqual" ||
+                tokens[2] == "OpFOrdGreaterThan" || tokens[2] == "OpFUnordGreaterThan") {
+                return tokens;
+            }
+        }
+        return {};
+    }
+
+    // Result id of the module's 0.0 constant of the type FAbs produces - the constant the source
+    // itself spelled - found without assuming what the disassembler names it or how it prints the
+    // literal.
+    String FindZeroConstantId(const String& disassembly) {
+        const Vector<Vector<String>> instructions = TokenizedInstructions(disassembly);
+        String floatTypeId;
+        for (const Vector<String>& tokens : instructions) {
+            if (tokens.size() >= 7 && tokens[2] == "OpExtInst" && tokens[5] == "FAbs") {
+                floatTypeId = tokens[3];
+                break;
+            }
+        }
+        if (floatTypeId.empty()) return {};
+
+        for (const Vector<String>& tokens : instructions) {
+            if (tokens.size() < 5 || tokens[2] != "OpConstant" || tokens[3] != floatTypeId) continue;
+            char* end = nullptr;
+            const double value = std::strtod(tokens[4].c_str(), &end);
+            if (end != nullptr && *end == '\0' && value == 0.0) return tokens[0];
+        }
+        return {};
+    }
+
+    // The shape the pass promises: the given opcode (either NaN half of it), tested against the
+    // module's own zero constant rather than against anything this pass invented.
+    void ExpectComparedAgainstModuleZero(const String& source, const String& orderedOpcode,
+                                         const String& unorderedOpcode) {
+        const String disassembly = OptimizedDisassembly(source);
+        const Vector<String> compare = FindRewrittenCompare(disassembly);
+        ASSERT_FALSE(compare.empty()) << "no rewritten compare in the optimized module\n"
+                                      << disassembly;
+        EXPECT_TRUE(compare[2] == orderedOpcode || compare[2] == unorderedOpcode)
+            << "expected " << orderedOpcode << " (or its unordered twin), got " << compare[2] << "\n"
+            << disassembly;
+
+        const String zeroId = FindZeroConstantId(disassembly);
+        ASSERT_FALSE(zeroId.empty()) << "the module has no 0.0 constant of the abs() type\n"
+                                     << disassembly;
+        EXPECT_EQ(compare.back(), zeroId)
+            << "the rewrite compares against " << compare.back()
+            << " instead of the module's own zero; a synthesized threshold is the epsilon bug\n"
+            << disassembly;
+    }
 } // namespace
 
 TEST_F(DemoteFloat64Test, AComparisonAgainstANonZeroDoubleIsLeftAlone) {
-    EXPECT_FALSE(RewritesToAnEpsilonTest(CompareAgainst("double", "1.0LF")))
+    EXPECT_FALSE(RewritesToAnAbsoluteValueTest(CompareAgainst("double", "1.0LF")))
-        << "a double compared against 1.0lf was rewritten into an epsilon test against zero";
+        << "a double compared against 1.0lf was rewritten into a test against zero";
 }
 
 TEST_F(DemoteFloat64Test, AComparisonAgainstZeroIsStillRewritten) {
-    EXPECT_TRUE(RewritesToAnEpsilonTest(CompareAgainst("double", "0.0LF")))
+    EXPECT_TRUE(RewritesToAnAbsoluteValueTest(CompareAgainst("double", "0.0LF")))
         << "the rewrite must still fire for a genuine comparison against zero";
 }
 
 TEST_F(DemoteFloat64Test, TheThirtyTwoBitBehaviourIsUnchanged) {
-    EXPECT_FALSE(RewritesToAnEpsilonTest(CompareAgainst("float", "1.0")))
+    EXPECT_FALSE(RewritesToAnAbsoluteValueTest(CompareAgainst("float", "1.0")))
         << "a float compared against 1.0 must not be rewritten";
-    EXPECT_TRUE(RewritesToAnEpsilonTest(CompareAgainst("float", "0.0")))
+    EXPECT_TRUE(RewritesToAnAbsoluteValueTest(CompareAgainst("float", "0.0")))
         << "the 32-bit behaviour this pass shipped with must be preserved exactly";
 }
+
+// The pass matches ZERO, not "small". The old constant-is-zero test was `fabs(v) <= 1e-4`, so a
+// float compared against exactly 1e-4 was declared a comparison against zero and rewritten into
+// `abs(x) >= 1e-4` - a different question from the one the shader asked, against a constant that
+// was never zero to begin with.
+TEST_F(DemoteFloat64Test, AComparisonAgainstASmallNonZeroLiteralIsLeftAlone) {
+    EXPECT_FALSE(RewritesToAnAbsoluteValueTest(CompareAgainst("float", "0.0001")))
+        << "a float compared against 1e-4 was treated as a comparison against zero";
+    EXPECT_FALSE(RewritesToAnAbsoluteValueTest(CompareAgainst("double", "0.0001LF")))
+        << "the 64-bit accessor must judge the constant just as exactly as the 32-bit one";
+}
+
+// What replaces the compare, not just that something did. Both properties here are what makes the
+// rewrite exact rather than a tolerance, and neither is visible in the FAbs fingerprint above.
+TEST_F(DemoteFloat64Test, TheRewriteComparesAbsAgainstTheModulesOwnZero) {
+    // `x == 0.0` -> `abs(x) <= 0.0`. The equality has to be INSIDE the replacement: with a strict
+    // `<` and no epsilon left to hide behind, +/-0 would stop comparing equal to zero.
+    ExpectComparedAgainstModuleZero(CompareAgainstUsing("float", "==", "0.0"),
+                                    "OpFOrdLessThanEqual", "OpFUnordLessThanEqual");
+    // `x != 0.0` -> `abs(x) > 0.0`, the strict complement of the above.
+    ExpectComparedAgainstModuleZero(CompareAgainstUsing("float", "!=", "0.0"), "OpFOrdGreaterThan",
+                                    "OpFUnordGreaterThan");
+}

MobileGL/MG_Util/ShaderTranspiler/SpirvPasses/EliminateFloatEqualsZeroPass.cpp

@@ -15,8 +15,6 @@
 #include "source/opt/ir_builder.h"
 #include "source/opt/ir_context.h"
 #include "source/opt/module.h"
-#include "source/opt/type_manager.h"
-#include <cmath>
 #include <vector>
 
 namespace MobileGL {
@@ -29,7 +27,6 @@ namespace MobileGL {
 
                 analysis::ConstantManager* const_mgr = context()->get_constant_mgr();
                 analysis::DefUseManager* def_use_mgr = context()->get_def_use_mgr();
-                analysis::TypeManager* type_mgr = context()->get_type_mgr();
 
                 // 2. Import `GLSL.std.450` extension ID (for abs() func)
                 uint32_t glsl_std_450_id = context()->get_feature_mgr()->GetExtInstImportId_GLSLstd450();
@@ -70,6 +67,10 @@ namespace MobileGL {
                             uint32_t op2_id = inst.GetSingleWordInOperand(1);
 
                             uint32_t var_id = 0;
+                            // The zero the source spelled, reused verbatim as the right-hand side
+                            // of the rewritten compare - so nothing has to be synthesized for a
+                            // width this pass would have to encode by hand.
+                            uint32_t zero_id = 0;
 
                             // The constant's WIDTH decides which accessor may read it, and asking
                             // the wrong one does not fail - it answers.
@@ -78,8 +79,8 @@ namespace MobileGL {
                             // bits. On a 64-bit constant words()[0] is the LOW half of the
                             // mantissa, and that half is zero for every round double a shader
                             // actually spells: 1.0lf, 2.0lf, 0.5lf, 100.0lf. Each of those
-                            // therefore looked like 0.0 here, and `d != 1.0lf` was rewritten into
+                            // therefore looked like 0.0 here, and `d != 1.0lf` was rewritten into a
-                            // `abs(d) >= epsilon` - which is TRUE for d == 1.0. That is the whole
+                            // test of `d` against ZERO - which is TRUE for d == 1.0. That is the whole
                             // of KHR-GL43.compute_shader.fp64-case2: twelve uniforms compared
                             // against vector and matrix constructors were untouched (a composite
                             // is not a FloatConstant) and the one scalar comparison in the shader
@@ -97,17 +98,24 @@ namespace MobileGL {
                                 const analysis::Float* floatType =
                                     floatConstant->type() != nullptr ? floatConstant->type()->AsFloat() : nullptr;
                                 if (floatType == nullptr) return false;
+                                // Exactly zero - a near-zero constant is not a zero constant.
+                                // `x == 1e-5` asks a different question than `x == 0.0` and must
+                                // keep its own right-hand side. -0.0 compares equal to 0.0 here,
+                                // which is correct: `x == -0.0` and `x == 0.0` are the same
+                                // predicate in IEEE, and abs() maps both zeroes onto +0.
                                 switch (floatType->width()) {
-                                case 32: return std::fabs(floatConstant->GetFloatValue()) <= K_EPSILON;
+                                case 32: return floatConstant->GetFloatValue() == 0.0f;
-                                case 64: return std::fabs(floatConstant->GetDoubleValue()) <= K_EPSILON;
+                                case 64: return floatConstant->GetDoubleValue() == 0.0;
                                 default: return false;
                                 }
                             };
 
                             if (is_float_zero(op2_id)) {
                                 var_id = op1_id; // x == 0.0
+                                zero_id = op2_id;
                             } else if (is_float_zero(op1_id)) {
                                 var_id = op2_id; // 0.0 == x
+                                zero_id = op1_id;
                             } else {
                                 ++itInst;
                                 continue;
@@ -120,12 +128,7 @@ namespace MobileGL {
                             uint32_t float_type_id = def_use_mgr->GetDef(var_id)->type_id();
                             uint32_t bool_type_id = inst.type_id();
 
-                            // 2. Create constant ID for `Epsilon`
+                            // 2. Build Abs(x) inst
-                            const analysis::Constant* eps_const = const_mgr->GetConstant(
-                                type_mgr->GetType(float_type_id), {*(reinterpret_cast<const uint32_t*>(&K_EPSILON))});
-                            uint32_t eps_id = const_mgr->GetDefiningInstruction(eps_const)->result_id();
-
-                            // 3. Build Abs(x) inst
                             // OpExtInst %float_type %glsl_import Abs %x
                             InstructionBuilder builder(
                                 context(), &inst, IRContext::kAnalysisDefUse | IRContext::kAnalysisInstrToBlockMapping);
@@ -140,38 +143,42 @@ namespace MobileGL {
                             Instruction* abs_inst = builder.AddInstruction(MakeUnique<Instruction>(
                                 context(), spv::Op::OpExtInst, float_type_id, context()->TakeNextId(), abs_operands));
 
-                            // 4. build Abs(x) < Epsilon
+                            // 3. build Abs(x) <= 0.0, or Abs(x) > 0.0 for the NotEqual forms
-                            // OpFOrdLessThan %bool_type %abs_val %eps
+                            // OpFOrdLessThanEqual %bool_type %abs_val %zero
-                            std::vector<Operand> less_operands;
+                            std::vector<Operand> cmp_operands;
-                            less_operands.push_back({SPV_OPERAND_TYPE_ID, {abs_inst->result_id()}});
+                            cmp_operands.push_back({SPV_OPERAND_TYPE_ID, {abs_inst->result_id()}});
-                            less_operands.push_back({SPV_OPERAND_TYPE_ID, {eps_id}});
+                            cmp_operands.push_back({SPV_OPERAND_TYPE_ID, {zero_id}});
 
+                            // Equality is INCLUDED in the replacement, which is what makes the
+                            // rewrite exact: |x| <= 0 is true for +0 and -0 and false for every
+                            // other finite value, |x| > 0 is its complement. The ordered/unordered
+                            // half of the opcode is preserved, so NaN keeps answering as it did.
                             spv::Op replacementOp = spv::Op::OpNop;
                             switch (inst.opcode()) {
                             case spv::Op::OpFOrdEqual:
-                                replacementOp = spv::Op::OpFOrdLessThan;
+                                replacementOp = spv::Op::OpFOrdLessThanEqual;
                                 break;
                             case spv::Op::OpFUnordEqual:
-                                replacementOp = spv::Op::OpFUnordLessThan;
+                                replacementOp = spv::Op::OpFUnordLessThanEqual;
                                 break;
                             case spv::Op::OpFOrdNotEqual:
-                                replacementOp = spv::Op::OpFOrdGreaterThanEqual;
+                                replacementOp = spv::Op::OpFOrdGreaterThan;
                                 break;
                             case spv::Op::OpFUnordNotEqual:
-                                replacementOp = spv::Op::OpFUnordGreaterThanEqual;
+                                replacementOp = spv::Op::OpFUnordGreaterThan;
                                 break;
                             default:
                                 MOBILEGL_ASSERT(false, "Unexpected float compare opcode: %d",
                                                 static_cast<int>(inst.opcode()));
                                 break;
                             }
-                            Instruction* less_than_inst = builder.AddInstruction(MakeUnique<Instruction>(
+                            Instruction* cmp_inst = builder.AddInstruction(MakeUnique<Instruction>(
-                                context(), replacementOp, bool_type_id, context()->TakeNextId(), less_operands));
+                                context(), replacementOp, bool_type_id, context()->TakeNextId(), cmp_operands));
 
-                            // 5. Replaces all uses of old insn with new one
+                            // 4. Replaces all uses of old insn with new one
-                            context()->ReplaceAllUsesWith(inst.result_id(), less_than_inst->result_id());
+                            context()->ReplaceAllUsesWith(inst.result_id(), cmp_inst->result_id());
 
-                            // 6. Kill old instruction (will be cleaned up by DCE later)
+                            // 5. Kill old instruction (will be cleaned up by DCE later)
                             auto nextInstIt = context()->KillInst(&inst);
                             if (nextInstIt) {
                                 itInst = nextInstIt;

MobileGL/MG_Util/ShaderTranspiler/SpirvPasses/EliminateFloatEqualsZeroPass.h

@@ -15,15 +15,28 @@
 namespace MobileGL {
     namespace MG_Util {
         namespace ShaderTranspiler {
+            // Keeps the driver's float-EQUALITY instruction out of the module: every scalar
+            // comparison against a constant 0.0 is re-spelled through GLSL.std.450 FAbs, so no
+            // OpFOrdEqual / OpFUnordEqual / OpFOrdNotEqual / OpFUnordNotEqual against zero ever
+            // reaches a shader compiler that gets exact float compare wrong.
+            //
+            // The rewrite is EXACT, not a tolerance. `x == 0.0` becomes `abs(x) <= 0.0` and
+            // `x != 0.0` becomes `abs(x) > 0.0`, both against the module's own zero constant:
+            // |x| <= 0 holds for +0 and -0 and for nothing else, so the two forms agree on every
+            // input, at any float width, with or without denormal flushing. The ordered/unordered
+            // half of the opcode is carried across unchanged, which is what keeps NaN answering
+            // the way it did before.
+            //
+            // It used to be an epsilon ball (abs(x) < 1e-4). That silently classified any
+            // legitimately small value as zero - KHR-GL3x.buffer_objects.triangles renders a
+            // specular term of ~6e-5 at a large render target and came out black - so the fuzz is
+            // gone; the reason the pass exists never needed it.
             class EliminateFloatEqualsZeroPass : public spvtools::opt::Pass {
             public:
                 const char* name() const override { return "float-equals-zero-elimination"; }
                 Status Process() override;
 
                 static spvtools::Optimizer::PassToken CreateEliminateFloatEqualsZeroPass();
-
-            private:
-                const float K_EPSILON = 0.0001f;
             };
         } // namespace ShaderTranspiler
     } // namespace MG_Util