From 392736fb6b5b876a1e6630c41c50824265cbfbe0 Mon Sep 17 00:00:00 2001 From: Swung0x48 Date: Thu, 20 Aug 2026 03:59:29 -0400 Subject: [PATCH] [Fix, Test] (ShaderTranspiler): rewrite float-equals-zero exactly instead of within a 1e-4 epsilon --- .../ShaderTranspiler/DemoteFloat64Test.cpp | 150 +++++++++++++++--- .../EliminateFloatEqualsZeroPass.cpp | 61 +++---- .../EliminateFloatEqualsZeroPass.h | 19 ++- 3 files changed, 182 insertions(+), 48 deletions(-) diff --git a/MobileGL/MG_Test/ShaderTranspiler/DemoteFloat64Test.cpp b/MobileGL/MG_Test/ShaderTranspiler/DemoteFloat64Test.cpp index 6128dd99..b1b6dfd0 100644 --- a/MobileGL/MG_Test/ShaderTranspiler/DemoteFloat64Test.cpp +++ b/MobileGL/MG_Test/ShaderTranspiler/DemoteFloat64Test.cpp @@ -8,6 +8,7 @@ #include +#include #include #include @@ -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 -// workaround for drivers whose exact float compare misbehaves. Deciding WHICH constants are -// zero used to read every float constant as though it were 32 bits wide, and on a 64-bit -// constant that reads the LOW half of the mantissa - which is zero for 1.0lf, 2.0lf, 0.5lf 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 -// uniform holding exactly 1.0. That is the whole of KHR-GL43.compute_shader.fp64-case2. +// EliminateFloatEqualsZeroPass re-spells a comparison against 0.0 through GLSL.std.450 FAbs, so +// that no float-equality instruction reaches a driver that gets one wrong. Deciding WHICH +// constants are zero used to read every float constant as though it were 32 bits wide, and on a +// 64-bit constant that reads the LOW half of the mantissa - which is zero for 1.0lf, 2.0lf, 0.5lf +// 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 a test against ZERO, and came out true for a uniform holding +// 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 input = CompileToSpirv(GL_COMPUTE_SHADER, source); EXPECT_FALSE(input.empty()); - if (input.empty()) return false; + if (input.empty()) return {}; Vector 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> TokenizedInstructions(const String& disassembly) { + Vector> instructions; + StringStream lines(disassembly); + String line; + while (std::getline(lines, line)) { + Vector 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 FindRewrittenCompare(const String& disassembly) { + for (const Vector& 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> instructions = TokenizedInstructions(disassembly); + String floatTypeId; + for (const Vector& tokens : instructions) { + if (tokens.size() >= 7 && tokens[2] == "OpExtInst" && tokens[5] == "FAbs") { + floatTypeId = tokens[3]; + break; + } + } + if (floatTypeId.empty()) return {}; + + for (const Vector& 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 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"))) - << "a double compared against 1.0lf was rewritten into an epsilon test against zero"; + EXPECT_FALSE(RewritesToAnAbsoluteValueTest(CompareAgainst("double", "1.0LF"))) + << "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"); +} diff --git a/MobileGL/MG_Util/ShaderTranspiler/SpirvPasses/EliminateFloatEqualsZeroPass.cpp b/MobileGL/MG_Util/ShaderTranspiler/SpirvPasses/EliminateFloatEqualsZeroPass.cpp index f16de06a..d58f3f32 100644 --- a/MobileGL/MG_Util/ShaderTranspiler/SpirvPasses/EliminateFloatEqualsZeroPass.cpp +++ b/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 #include 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 - // `abs(d) >= epsilon` - which is TRUE for d == 1.0. That is the whole + // therefore looked like 0.0 here, and `d != 1.0lf` was rewritten into a + // 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 64: return std::fabs(floatConstant->GetDoubleValue()) <= K_EPSILON; + case 32: return floatConstant->GetFloatValue() == 0.0f; + 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` - const analysis::Constant* eps_const = const_mgr->GetConstant( - type_mgr->GetType(float_type_id), {*(reinterpret_cast(&K_EPSILON))}); - uint32_t eps_id = const_mgr->GetDefiningInstruction(eps_const)->result_id(); - - // 3. Build Abs(x) inst + // 2. 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( context(), spv::Op::OpExtInst, float_type_id, context()->TakeNextId(), abs_operands)); - // 4. build Abs(x) < Epsilon - // OpFOrdLessThan %bool_type %abs_val %eps - std::vector less_operands; - less_operands.push_back({SPV_OPERAND_TYPE_ID, {abs_inst->result_id()}}); - less_operands.push_back({SPV_OPERAND_TYPE_ID, {eps_id}}); + // 3. build Abs(x) <= 0.0, or Abs(x) > 0.0 for the NotEqual forms + // OpFOrdLessThanEqual %bool_type %abs_val %zero + std::vector cmp_operands; + cmp_operands.push_back({SPV_OPERAND_TYPE_ID, {abs_inst->result_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(inst.opcode())); break; } - Instruction* less_than_inst = builder.AddInstruction(MakeUnique( - context(), replacementOp, bool_type_id, context()->TakeNextId(), less_operands)); + Instruction* cmp_inst = builder.AddInstruction(MakeUnique( + context(), replacementOp, bool_type_id, context()->TakeNextId(), cmp_operands)); - // 5. Replaces all uses of old insn with new one - context()->ReplaceAllUsesWith(inst.result_id(), less_than_inst->result_id()); + // 4. Replaces all uses of old insn with new one + 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; diff --git a/MobileGL/MG_Util/ShaderTranspiler/SpirvPasses/EliminateFloatEqualsZeroPass.h b/MobileGL/MG_Util/ShaderTranspiler/SpirvPasses/EliminateFloatEqualsZeroPass.h index 8e6ab37d..6c59a225 100644 --- a/MobileGL/MG_Util/ShaderTranspiler/SpirvPasses/EliminateFloatEqualsZeroPass.h +++ b/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