Module: Mesa Branch: staging/22.1 Commit: 44dd0ef9ecca10b237ab7d5dca58222b467c183d URL: http://cgit.freedesktop.org/mesa/mesa/commit/?id=44dd0ef9ecca10b237ab7d5dca58222b467c183d
Author: Emma Anholt <[email protected]> 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> (cherry picked from commit 43579901be327527517633620d64103ee0365d92) --- .pick_status.json | 2 +- src/freedreno/ir3/ir3_nir_lower_tess.c | 49 ++++++++++++++++++---------------- 2 files changed, 27 insertions(+), 24 deletions(-) diff --git a/.pick_status.json b/.pick_status.json index 39b5b35183e..1a56e0025c2 100644 --- a/.pick_status.json +++ b/.pick_status.json @@ -5944,7 +5944,7 @@ "description": "ir3: Fix the no-emitted-vertex condition emission in geom lowering.", "nominated": true, "nomination_type": 0, - "resolution": 0, + "resolution": 1, "main_sha": null, "because_sha": null }, diff --git a/src/freedreno/ir3/ir3_nir_lower_tess.c b/src/freedreno/ir3/ir3_nir_lower_tess.c index 63bb3e24c18..becf51e1087 100644 --- a/src/freedreno/ir3/ir3_nir_lower_tess.c +++ b/src/freedreno/ir3/ir3_nir_lower_tess.c @@ -984,31 +984,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);
