nir/from_ssa: Only chain movs when a src is also a dest

The algorithm we use for resolving parallel copy instructions plays this
little shell game with the values.  The reason for this is that it lets
us handle cases where, for instance we have a -> b and b -> a and we
need to use a temporary to do a swap.  One result of this algorithm is
that it tends to emit a lot of mov chains which are typcially really bad
for GPUs where a mov is far from free.  For instance, it's likely to
turn this:

    r16 = ssa_0; r17 = ssa_0; r18 = ssa_0; r15 = ssa_0

into this:

    r15 = mov ssa_0
    r18 = mov r15
    r17 = mov r18
    r16 = mov r17

which, if it's the only thing in a block (this is common for phis) is
impossible for a scheduler to fix because of the dependencies and you
end up with significant stalling.  If, on the other hand, we only do the
chaining in the actual case where we need to free up a so that it can be
used as a destination, we can emit this:

    r15 = mov ssa_0
    r18 = mov ssa_0
    r17 = mov ssa_0
    r16 = mov ssa_0

which is far nicer to the scheduler.  On Intel, our copy propagation
pass will undo the chain for us so this has no shader-db impact.
However, for less intelligent back-ends, it's probably a lot better.

Reviewed-by: Rob Clark <robdclark@chromium.org>
Reviewed-by: Connor Abbott <cwabbott0@gmail.com>
Tested-by: Marge Bot <https://gitlab.freedesktop.org/mesa/mesa/-/merge_requests/4412>
Part-of: <https://gitlab.freedesktop.org/mesa/mesa/-/merge_requests/4412>

diff --git a/src/compiler/nir/nir_from_ssa.c b/src/compiler/nir/nir_from_ssa.c
index acb758584a6dbdf62ab5d2b1e6d28c6becaa44de..41ee4e795812db03b9ce8337a4e73df5d935ea50 100644 (file)
--- a/src/compiler/nir/nir_from_ssa.c
+++ b/src/compiler/nir/nir_from_ssa.c
@@ -680,15 +680,17 @@ resolve_parallel_copy(nir_parallel_copy_instr *pcopy,
          int a = pred[b];
          emit_copy(&state->builder, values[loc[a]], values[b]);
 
-         /* If any other copies want a they can find it at b */
-         loc[a] = b;
-
          /* b has been filled, mark it as not needing to be copied */
          pred[b] = -1;
 
-         /* If a needs to be filled, it's ready for copying now */
-         if (pred[a] != -1)
+         /* If a needs to be filled... */
+         if (pred[a] != -1) {
+            /* If any other copies want a they can find it at b */
+            loc[a] = b;
+
+            /* It's ready for copying now */
             ready[++ready_idx] = a;
+         }
       }
       int b = to_do[to_do_idx--];
       if (pred[b] == -1)