Mesa (main): ir3: Fix the no-emitted-vertex condition emission in geom lowering.

[GitLab Mirror]

Module: Mesa
Branch: main
Commit: 43579901be327527517633620d64103ee0365d92
URL:    http://cgit.freedesktop.org/mesa/mesa/commit/?id=43579901be327527517633620d64103ee0365d92

Author: Emma Anholt <emma at anholt.net>
Date:   Tue Jul 12 16:10:17 2022 -0700

ir3: Fix the no-emitted-vertex condition emission in geom lowering.

The if statement we insert would insert a new block before the end block
(and remove the old pre-end-block).  If the new block ended up later in
the HT due to its pointer's hash value, you'd emit another copy of the if
statement after the last one.  I saw this happen up to 4 times in testing.
The worst case would be if all those additions and removals ended up
reallocating the HT, at which point we might use-after-free.

Fixes inconsistent shader-db results with geometry shaders.

Cc: mesa-stable.
Part-of: <https://gitlab.freedesktop.org/mesa/mesa/-/merge_requests/17501>

---

 src/freedreno/ir3/ir3_nir_lower_tess.c | 49 ++++++++++++++++++----------------
 1 file changed, 26 insertions(+), 23 deletions(-)

diff --git a/src/freedreno/ir3/ir3_nir_lower_tess.c b/src/freedreno/ir3/ir3_nir_lower_tess.c
index 1e0b217993c..ae99ae85022 100644
--- a/src/freedreno/ir3/ir3_nir_lower_tess.c
+++ b/src/freedreno/ir3/ir3_nir_lower_tess.c
@@ -1004,31 +1004,34 @@ ir3_nir_lower_gs(nir_shader *shader)
    nir_foreach_block_safe (block, impl)
       lower_gs_block(block, &b, &state);
 
-   set_foreach (impl->end_block->predecessors, block_entry) {
-      struct nir_block *block = (void *)block_entry->key;
-      b.cursor = nir_after_block_before_jump(block);
-
-      nir_ssa_def *cond =
-         nir_ieq_imm(&b, nir_load_var(&b, state.emitted_vertex_var), 0);
-
-      /* If we haven't emitted any vertex we need to copy the shadow (old)
-       * outputs to emit outputs here.
-       *
-       * Also some piglit GS tests[1] don't have EndPrimitive() so throw
-       * in an extra vertex_flags write for good measure.  If unneeded it
-       * will be optimized out.
-       *
-       * [1] ex, tests/spec/glsl-1.50/execution/compatibility/clipping/gs-clip-vertex-const-accept.shader_test
-       */
-      nir_push_if(&b, cond);
-      nir_store_var(&b, state.vertex_flags_out, nir_imm_int(&b, 4), 0x1);
-      copy_vars(&b, &state.emit_outputs, &state.old_outputs);
-      nir_pop_if(&b, NULL);
+   /* Note: returns are lowered, so there should be only one block before the
+    * end block.  If we had real returns, we would probably want to redirect
+    * them to this new if statement, rather than emitting this code at every
+    * return statement.
+    */
+   assert(impl->end_block->predecessors->entries == 1);
+   nir_block *block = nir_impl_last_block(impl);
+   b.cursor = nir_after_block_before_jump(block);
+
+   /* If we haven't emitted any vertex we need to copy the shadow (old)
+    * outputs to emit outputs here.
+    *
+    * Also some piglit GS tests[1] don't have EndPrimitive() so throw
+    * in an extra vertex_flags write for good measure.  If unneeded it
+    * will be optimized out.
+    *
+    * [1] ex, tests/spec/glsl-1.50/execution/compatibility/clipping/gs-clip-vertex-const-accept.shader_test
+    */
+   nir_ssa_def *cond =
+      nir_ieq_imm(&b, nir_load_var(&b, state.emitted_vertex_var), 0);
+   nir_push_if(&b, cond);
+   nir_store_var(&b, state.vertex_flags_out, nir_imm_int(&b, 4), 0x1);
+   copy_vars(&b, &state.emit_outputs, &state.old_outputs);
+   nir_pop_if(&b, NULL);
 
-      nir_discard_if(&b, cond);
+   nir_discard_if(&b, cond);
 
-      copy_vars(&b, &state.new_outputs, &state.emit_outputs);
-   }
+   copy_vars(&b, &state.new_outputs, &state.emit_outputs);
 
    exec_list_append(&shader->variables, &state.old_outputs);
    exec_list_append(&shader->variables, &state.emit_outputs);