intel/fs: Don't try to copy propagate into a source again after progress is made
authorIan Romanick <ian.d.romanick@intel.com>
Fri, 18 Aug 2023 16:34:35 +0000 (09:34 -0700)
committerMarge Bot <emma+marge@anholt.net>
Thu, 14 Sep 2023 22:31:22 +0000 (22:31 +0000)
If the linked list structure used depended on the list head to know when
to terminate, this would be a pretty serious bug. If try_constant_propage
or try_copy_propagate make progress, inst->src[i].nr will change. This
results in the foreach_in_list using a different list header on later
iterations of the loop.

This causes two shaders in shader-db and 9 shaders in fossil-db to
change. Looking at the code changes, these are cases where there was a
copy of a copy that gets propagated. The part that confuses me is the
VGRF numbers involved should **not** hash to the same bucket, so it
should be impossible to find the original source from the intermediate
VGRF.

Reviewed-by: Kenneth Graunke <kenneth@whitecape.org>
Part-of: <https://gitlab.freedesktop.org/mesa/mesa/-/merge_requests/25091>

src/intel/compiler/brw_fs_copy_propagation.cpp

index 8151b7d..00443e0 100644 (file)
@@ -1181,10 +1181,13 @@ fs_visitor::opt_copy_propagation_local(void *copy_prop_ctx, bblock_t *block,
             continue;
 
          foreach_in_list(acp_entry, entry, &acp[inst->src[i].nr % ACP_HASH_SIZE]) {
-            if (try_constant_propagate(inst, entry))
+            if (try_constant_propagate(inst, entry)) {
                progress = true;
-            else if (try_copy_propagate(inst, i, entry))
+               break;
+            } else if (try_copy_propagate(inst, i, entry)) {
                progress = true;
+               break;
+            }
          }
       }