Mesa (staging/20.0): intel/fs: Correctly handle multiply of fsign with a source modifier

[GitLab Mirror]

Module: Mesa
Branch: staging/20.0
Commit: 01020aef25cca94713ac860256d2eb6296b45d1a
URL:    http://cgit.freedesktop.org/mesa/mesa/commit/?id=01020aef25cca94713ac860256d2eb6296b45d1a

Author: Ian Romanick <ian.d.romanick at intel.com>
Date:   Tue Feb 11 12:00:00 2020 -0800

intel/fs: Correctly handle multiply of fsign with a source modifier

The other source of the multiply will be interpreted as a uint32_t in an
XOR instruction.  Any source modifiers with either not be interpreted at
all or will be misinterpreted due to the differing types.

If the other operand of the multiplication has a source modifier, just
emit an extra move to resolve the source modifiers.

The negation source modifier problem is difficult to reproduce due to an
algebraic optimization that changes (-a*b) to -(a*b).  However, changes
in MR !1359 push the negations back down.

On Gen7+ it might be possible to do slightly better for an abs() source
modifier by using BFI2 as a glorified copysign().

On Gen8+ it might be possible to do slightly better for a neg() source
modifier by emitting (~a ^ b).

There were no shader-db changes on any Intel platform, so I think we can
deal with that problem when it arises.

See also piglit!224.

Fixes: 06d2c116415 ("intel/fs: Add a scale factor to emit_fsign")
Reviewed-by: Matt Turner <mattst88 at gmail.com>
Tested-by: Marge Bot <https://gitlab.freedesktop.org/mesa/mesa/merge_requests/3780>
Part-of: <https://gitlab.freedesktop.org/mesa/mesa/merge_requests/3780>
(cherry picked from commit 273b8cd1ca286e2f43b4a464a391fdcaac49f077)

---

 .pick_status.json                 |  2 +-
 src/intel/compiler/brw_fs_nir.cpp | 10 ++++++++++
 2 files changed, 11 insertions(+), 1 deletion(-)

diff --git a/.pick_status.json b/.pick_status.json
index b5951ca51b2..409d0b76d92 100644
--- a/.pick_status.json
+++ b/.pick_status.json
@@ -76,7 +76,7 @@
         "description": "intel/fs: Correctly handle multiply of fsign with a source modifier",
         "nominated": true,
         "nomination_type": 1,
-        "resolution": 0,
+        "resolution": 1,
         "master_sha": null,
         "because_sha": "06d2c116415c0ab163a57ed7f2522342ed43e4d4"
     },
diff --git a/src/intel/compiler/brw_fs_nir.cpp b/src/intel/compiler/brw_fs_nir.cpp
index fab27c10d8c..6e9bf55501d 100644
--- a/src/intel/compiler/brw_fs_nir.cpp
+++ b/src/intel/compiler/brw_fs_nir.cpp
@@ -881,6 +881,16 @@ fs_visitor::emit_fsign(const fs_builder &bld, const nir_alu_instr *instr,
       }
 
       op[0] = offset(op[0], bld, fsign_instr->src[0].swizzle[channel]);
+
+      /* Resolve any source modifiers.  We could do slightly better on Gen8+
+       * if the only source modifier is negation, but *shrug*.
+       */
+      if (op[1].negate || op[1].abs) {
+         fs_reg tmp = bld.vgrf(op[1].type);
+
+         bld.MOV(tmp, op[1]);
+         op[1] = tmp;
+      }
    } else {
       assert(!instr->dest.saturate);
    }