From 3d8fea0e092e66abcac466ea630e7dcf42c76db8 Mon Sep 17 00:00:00 2001 From: Ian Romanick Date: Tue, 25 Jun 2024 13:14:58 -0700 Subject: [PATCH] intel/brw: Don't propagate saturate to an instruction that writes flags There are two problems. 1. This is not NaN safe. 'add.le.sat dst F, Inf F, -Inf F' has a different result than 'add dst F, Inf F, -Inf F; cmp.le null, dst F, 0F'. 2. Ignoring the first problem, this only produces the desired flags for LE and G. All other cases can produce the wrong result. For example, batman_arkham_city_goty.foz 6a63c4caacaa0dae has the following code: mad.ge.f0.0(8) g51<1>F g50<8,8,1>F g46<8,8,1>F g11<1,1,1>F mov.sat(8) g52<1>F g51<1,1,0>F ... (+f0.0) sel(8) g54<1>UD g53<8,8,1>UD 0x3f000000UD Without this commit, the saturate is incorrectly propagated to the MAD. A similar case exists in witcher_3_dxvk_g2.foz 5b03243be667a275. There are even worse cases like total_war_warhammer3.dx12vk-g6.foz 78328466761ef7ab and ee920491573860fc. The former has the following code (and the latter has very similar code): mad.l.f0.0(16) g95<1>F g93<8,8,1>F g62<8,8,1>F g68<1,1,1>F ... mov.sat(16) g109<1>F -g95<1,1,0>F ... (+f0.0) sel(16) g68<1>UD g111<1,1,0>UD g54<1,1,0>UD (+f0.0) sel(16) g70<1>UD g113<1,1,0>UD g56<1,1,0>UD (+f0.0) sel(16) g72<1>UD g115<1,1,0>UD g58<1,1,0>UD Saturate propagation makes a hash of this code: mad.sat.l.f0.0(16) g106<1>F -g93<8,8,1>F -g62<8,8,1>F g68<1,1,1>F ... (+f0.0) sel(16) g70<1>UD g110<1,1,0>UD g56<1,1,0>UD (+f0.0) sel(16) g72<1>UD g112<1,1,0>UD g58<1,1,0>UD (+f0.0) sel(16) g68<1>UD g108<1,1,0>UD g54<1,1,0>UD Not only is the saturate incorrectly applied to the MAD, but the MAD result is negated without changing the conditional modifier to G! NOTE: Backports of this commit to stable branches may need to be more like the following commit to elk. shader-db: All Intel platforms had similar results. (Meteor Lake shown) total instructions in shared programs: 19729375 -> 19729377 (<.01%) instructions in affected programs: 112 -> 114 (1.79%) helped: 0 HURT: 2 total cycles in shared programs: 916234266 -> 916234288 (<.01%) cycles in affected programs: 636 -> 658 (3.46%) helped: 0 HURT: 2 fossil-db: All Intel platforms had similar results. (Meteor Lake shown) Totals: Instrs: 151531594 -> 151531601 (+0.00%) Cycle count: 17209107419 -> 17209107474 (+0.00%); split: -0.00%, +0.00% Totals from 6 (0.00% of 630198) affected shaders: Instrs: 4550 -> 4557 (+0.15%) Cycle count: 194629 -> 194684 (+0.03%); split: -0.00%, +0.03% Fixes: 947c828d5cb ("i965/fs: Add a saturation propagation optimization pass.") Reviewed-by: Kenneth Graunke Part-of: --- src/intel/compiler/brw_fs_saturate_propagation.cpp | 3 +++ 1 file changed, 3 insertions(+) diff --git a/src/intel/compiler/brw_fs_saturate_propagation.cpp b/src/intel/compiler/brw_fs_saturate_propagation.cpp index 1abd5411931..3d8b3e68a84 100644 --- a/src/intel/compiler/brw_fs_saturate_propagation.cpp +++ b/src/intel/compiler/brw_fs_saturate_propagation.cpp @@ -75,6 +75,9 @@ opt_saturate_propagation_local(fs_visitor &s, bblock_t *block) !scan_inst->can_change_types())) break; + if (scan_inst->flags_written(s.devinfo) != 0) + break; + if (scan_inst->saturate) { inst->saturate = false; progress = true;