fdo-mirrors/mesa
1
0
Fork 0
mirror of https://gitlab.freedesktop.org/mesa/mesa.git synced 2025-12-24 08:50:13 +01:00
Code Issues Projects Releases Packages Wiki Activity Actions 1

intel/brw: Don't propagate saturate to an instruction that writes flags

Browse source 
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: 947c828d5c ("i965/fs: Add a saturation propagation optimization pass.")
Reviewed-by: Kenneth Graunke <kenneth@whitecape.org>
Part-of: <https://gitlab.freedesktop.org/mesa/mesa/-/merge_requests/29774>
This commit is contained in:
Ian Romanick 2024-06-25 13:14:58 -07:00
parent 6da4649191
commit 3d8fea0e09
1 changed files with 3 additions and 0 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

3
src/intel/compiler/brw_fs_saturate_propagation.cpp
View file

@ -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;