From 829e5ccc846c7df9ff7a0bcfa74e8e9861bc9ce2 Mon Sep 17 00:00:00 2001 From: Ian Romanick Date: Fri, 6 Mar 2026 14:44:47 -0800 Subject: [PATCH] brw/algebraic: Don't optimize SEL.L.SAT or SEL.G.SAT This optimization was added in October 2013, and the error was only just now discovered. Removing the SEL.G.SAT optimization affected zero shader-db shaders, and it affected 9 fossil-db shaders for instruction size only. I haven't checked to see if any of the hurt shaders are helped by !39987. shader-db: All Intel platforms had similar results. (Lunar Lake shown) total instructions in shared programs: 17093041 -> 17093055 (<.01%) instructions in affected programs: 2072 -> 2086 (0.68%) helped: 0 / HURT: 8 total cycles in shared programs: 876739578 -> 876739154 (<.01%) cycles in affected programs: 18946 -> 18522 (-2.24%) helped: 2 / HURT: 6 fossil-db: Lunar Lake Totals: Instrs: 906230557 -> 906240487 (+0.00%); split: -0.00%, +0.00% CodeSize: 14498856128 -> 14499003168 (+0.00%); split: -0.00%, +0.00% Send messages: 40667184 -> 40667205 (+0.00%); split: -0.00%, +0.00% Cycle count: 104068494103 -> 104068561943 (+0.00%); split: -0.00%, +0.00% Max live registers: 189570192 -> 189570204 (+0.00%); split: -0.00%, +0.00% Max dispatch width: 48157648 -> 48157552 (-0.00%) Non SSA regs after NIR: 139823587 -> 139823016 (-0.00%); split: -0.00%, +0.00% Totals from 9172 (0.46% of 1985212) affected shaders: Instrs: 10774709 -> 10784639 (+0.09%); split: -0.00%, +0.09% CodeSize: 177868384 -> 178015424 (+0.08%); split: -0.08%, +0.17% Send messages: 311154 -> 311175 (+0.01%); split: -0.00%, +0.01% Cycle count: 232471392 -> 232539232 (+0.03%); split: -0.15%, +0.18% Max live registers: 1243549 -> 1243561 (+0.00%); split: -0.00%, +0.01% Max dispatch width: 196672 -> 196576 (-0.05%) Non SSA regs after NIR: 509663 -> 509092 (-0.11%); split: -0.19%, +0.08% Test: piglit!1100 Reported-by: Georg Lehmann Fixes: ca675b73d3a ("i965/fs: Optimize saturating SEL.L(E) with imm val >= 1.0.") Reviewed-by: Caio Oliveira (cherry picked from commit 6c6c6ce054dcd3cd9de81a840b6f4443eba3085c) Part-of: --- .pick_status.json | 2 +- src/intel/compiler/brw/brw_opt_algebraic.cpp | 33 ------------------- src/intel/compiler/brw/test_opt_algebraic.cpp | 21 ++++++++++++ 3 files changed, 22 insertions(+), 34 deletions(-) diff --git a/.pick_status.json b/.pick_status.json index 326a95e0de1..75db7b2dc36 100644 --- a/.pick_status.json +++ b/.pick_status.json @@ -1144,7 +1144,7 @@ "description": "brw/algebraic: Don't optimize SEL.L.SAT or SEL.G.SAT", "nominated": true, "nomination_type": 2, - "resolution": 0, + "resolution": 1, "main_sha": null, "because_sha": "ca675b73d3ac2e1b57ec385c2c80b05b6382f6b6", "notes": null diff --git a/src/intel/compiler/brw/brw_opt_algebraic.cpp b/src/intel/compiler/brw/brw_opt_algebraic.cpp index 4a17a5855cc..ff1932e1def 100644 --- a/src/intel/compiler/brw/brw_opt_algebraic.cpp +++ b/src/intel/compiler/brw/brw_opt_algebraic.cpp @@ -553,39 +553,6 @@ brw_opt_algebraic(brw_shader &s) inst->predicate_inverse = false; inst->conditional_mod = BRW_CONDITIONAL_NONE; progress = true; - } else if (inst->saturate && inst->src[1].file == IMM) { - switch (inst->conditional_mod) { - case BRW_CONDITIONAL_LE: - case BRW_CONDITIONAL_L: - switch (inst->src[1].type) { - case BRW_TYPE_F: - if (inst->src[1].f >= 1.0f) { - inst = brw_transform_inst(s, inst, BRW_OPCODE_MOV); - inst->conditional_mod = BRW_CONDITIONAL_NONE; - progress = true; - } - break; - default: - break; - } - break; - case BRW_CONDITIONAL_GE: - case BRW_CONDITIONAL_G: - switch (inst->src[1].type) { - case BRW_TYPE_F: - if (inst->src[1].f <= 0.0f) { - inst = brw_transform_inst(s, inst, BRW_OPCODE_MOV); - inst->conditional_mod = BRW_CONDITIONAL_NONE; - progress = true; - } - break; - default: - break; - } - break; - default: - break; - } } break; case BRW_OPCODE_CSEL: diff --git a/src/intel/compiler/brw/test_opt_algebraic.cpp b/src/intel/compiler/brw/test_opt_algebraic.cpp index 397b6aefd90..68478600425 100644 --- a/src/intel/compiler/brw/test_opt_algebraic.cpp +++ b/src/intel/compiler/brw/test_opt_algebraic.cpp @@ -59,6 +59,27 @@ TEST_F(algebraic_test, fmax_a_a) EXPECT_NO_PROGRESS(brw_opt_algebraic, bld); } +TEST_F(algebraic_test, fmin_sat_a_1) +{ + brw_builder bld = make_shader(MESA_SHADER_FRAGMENT, 16); + + brw_reg dst0 = vgrf(bld, BRW_TYPE_F); + brw_reg src0 = vgrf(bld, BRW_TYPE_F); + + /* This is NaN. The definition of NAN in math.h has a scary comment about + * possibly raising an exception outside static initializers. + */ + bld.MOV(retype(src0, BRW_TYPE_UD), brw_imm_ud(0x7fc00000)); + bld.emit_minmax(dst0, src0, brw_imm_f(1.0), BRW_CONDITIONAL_L) + ->saturate = true; + + /* SEL.L of NaN should produce 1.0, and the .SAT should occur + * afterwards. This means the SEL.L.SAT cannot be optimized to a simple + * MOV.SAT since that would saturate NaN to 0.0 instead. + */ + EXPECT_NO_PROGRESS(brw_opt_algebraic, bld); +} + TEST_F(algebraic_test, mad_with_all_immediates) { brw_builder bld = make_shader();