From 7e127c1fca6bd934469f4803dde54842fbd100da Mon Sep 17 00:00:00 2001 From: Ian Romanick Date: Tue, 12 Jan 2021 12:23:16 -0800 Subject: [PATCH] nir/algebraic: Fix some min/max of b2f replacements MIME-Version: 1.0 Content-Type: text/plain; charset=utf8 Content-Transfer-Encoding: 8bit fmin(-A, -B) is -fmax(A, B), and fmax(-A, -B) is -fmin(A, B). Therefore the logic joining A and B should toggle between ior and iand for the negated versions. At the very least, a shader from Euro Truck Simulator 2 in shader-db is affected by this. The KIL instruction in the (ARB assembly) shader ends up with the wrong logic. This is _probably_ the source of https://gitlab.freedesktop.org/mesa/mesa/-/issues/1346. That said, the issue mentions that Mesa 18.0.5 works, but commit 68420d8322c ("nir: Simplify min and max of b2f") was added in 17.3. Moreover, I was not able to reproduce the error in the ETS2 shader from shader-db from any Mesa commit near the time the original fd.o bugzilla was submitted (December 2018). :shrug: In fact, the current error in that shader starts with 9167324a86b ("nir/algebraic: Mark some logic-joined comparison reductions as exact"). That's a bit of a red herring as 9167324a86b just sets off a chain of replacements that eventually leads to the incorrect min/max of b2f patterns fixed by this commit. The other affected shaders in the shader-db results are from Cargo Commander. These are also ARB assembly shaders. I think any ARB assembly shader that uses the pattern SLT r0, ...; ... KIL -r0; will suffer from issues related to this. This change fixes the piglit tests/spec/arb_fragment_program/kil-of-slt.shader_test test added in https://gitlab.freedesktop.org/mesa/piglit/-/merge_requests/454. shader-db results: All Gen6+ platforms had similar result. (Ice Lake shown) total instructions in shared programs: 20034604 -> 20034486 (<.01%) instructions in affected programs: 3885 -> 3767 (-3.04%) helped: 47 HURT: 2 helped stats (abs) min: 2 max: 4 x̄: 2.64 x̃: 2 helped stats (rel) min: 2.33% max: 8.33% x̄: 3.48% x̃: 3.39% HURT stats (abs) min: 3 max: 3 x̄: 3.00 x̃: 3 HURT stats (rel) min: 13.64% max: 16.67% x̄: 15.15% x̃: 15.15% 95% mean confidence interval for instructions value: -2.83 -1.99 95% mean confidence interval for instructions %-change: -3.84% -1.60% Instructions are helped. total cycles in shared programs: 979881379 -> 979879406 (<.01%) cycles in affected programs: 119873 -> 117900 (-1.65%) helped: 46 HURT: 3 helped stats (abs) min: 10 max: 756 x̄: 45.41 x̃: 26 helped stats (rel) min: 0.53% max: 19.72% x̄: 1.67% x̃: 1.26% HURT stats (abs) min: 28 max: 56 x̄: 38.67 x̃: 32 HURT stats (rel) min: 1.44% max: 3.54% x̄: 2.75% x̃: 3.27% 95% mean confidence interval for cycles value: -70.83 -9.70 95% mean confidence interval for cycles %-change: -2.23% -0.57% Cycles are helped. Iron Lake and GM45 had similar results. (Iron Lake shown) total instructions in shared programs: 8115098 -> 8115076 (<.01%) instructions in affected programs: 2592 -> 2570 (-0.85%) helped: 32 HURT: 2 helped stats (abs) min: 1 max: 1 x̄: 1.00 x̃: 1 helped stats (rel) min: 0.88% max: 2.70% x̄: 1.35% x̃: 1.31% HURT stats (abs) min: 5 max: 5 x̄: 5.00 x̃: 5 HURT stats (rel) min: 17.24% max: 18.52% x̄: 17.88% x̃: 17.88% 95% mean confidence interval for instructions value: -1.15 -0.15 95% mean confidence interval for instructions %-change: -1.83% 1.39% Inconclusive result (%-change mean confidence interval includes 0). total cycles in shared programs: 238189718 -> 238189802 (<.01%) cycles in affected programs: 75076 -> 75160 (0.11%) helped: 3 HURT: 31 helped stats (abs) min: 2 max: 130 x̄: 44.67 x̃: 2 helped stats (rel) min: 0.18% max: 5.70% x̄: 2.02% x̃: 0.19% HURT stats (abs) min: 2 max: 70 x̄: 7.03 x̃: 4 HURT stats (rel) min: 0.07% max: 6.41% x̄: 0.53% x̃: 0.15% 95% mean confidence interval for cycles value: -7.27 12.21 95% mean confidence interval for cycles %-change: -0.33% 0.94% Inconclusive result (value mean confidence interval includes 0). No fossil-db changes on any Intel platform. Fixes: 68420d8322c ("nir: Simplify min and max of b2f") Closes: #1346 Reviewed-by: Matt Turner Part-of: --- src/compiler/nir/nir_opt_algebraic.py | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/src/compiler/nir/nir_opt_algebraic.py b/src/compiler/nir/nir_opt_algebraic.py index 1f12ccb..87dce5d 100644 --- a/src/compiler/nir/nir_opt_algebraic.py +++ b/src/compiler/nir/nir_opt_algebraic.py @@ -512,10 +512,11 @@ optimizations.extend([ # a != fsat(a) (('ior', ('flt', a, 0.0), ('flt', 1.0, a)), ('fneu', a, ('fsat', a)), '!options->lower_fsat'), + # Note: fmin(-a, -b) == -fmax(a, b) (('fmax', ('b2f(is_used_once)', 'a@1'), ('b2f', 'b@1')), ('b2f', ('ior', a, b))), - (('fmax', ('fneg(is_used_once)', ('b2f(is_used_once)', 'a@1')), ('fneg', ('b2f', 'b@1'))), ('fneg', ('b2f', ('ior', a, b)))), + (('fmax', ('fneg(is_used_once)', ('b2f(is_used_once)', 'a@1')), ('fneg', ('b2f', 'b@1'))), ('fneg', ('b2f', ('iand', a, b)))), (('fmin', ('b2f(is_used_once)', 'a@1'), ('b2f', 'b@1')), ('b2f', ('iand', a, b))), - (('fmin', ('fneg(is_used_once)', ('b2f(is_used_once)', 'a@1')), ('fneg', ('b2f', 'b@1'))), ('fneg', ('b2f', ('iand', a, b)))), + (('fmin', ('fneg(is_used_once)', ('b2f(is_used_once)', 'a@1')), ('fneg', ('b2f', 'b@1'))), ('fneg', ('b2f', ('ior', a, b)))), # fmin(b2f(a), b) # bcsel(a, fmin(b2f(a), b), fmin(b2f(a), b)) -- 2.7.4