From 3ef858a6f6789207e3f24550e9dfb595e3018029 Mon Sep 17 00:00:00 2001 From: Connor Abbott Date: Wed, 24 Nov 2021 17:51:19 +0100 Subject: [PATCH] ir3/spill: Fix simplify_phi_nodes with multiple loop nesting Once we simplified a phi node, we never updated the definition it points to, which meant that it could become out of date if that definition were also simplified, and we didn't check that when rewriting sources. That could happen when there are multiple nested loops with phi nodes at the header. Fix it by updating the phi's pointer. Since we always update sources after visiting the definition it points to, when we go to rewrite a source, if that source points to a simplified phi, the phi's pointer can't be pointing to a simplified phi because we already visited the phi earlier in the pass and updated it, or else it's been simplified in the meantime and this isn't the last pass. This way we don't need to keep recursing when rewriting sources. Fixes: 613eaac7b53 ("ir3: Initial support for spilling non-shared registers") Part-of: --- src/freedreno/ci/freedreno-a618-fails.txt | 4 --- src/freedreno/ci/freedreno-a630-fails.txt | 4 --- src/freedreno/ir3/ir3_spill.c | 42 +++++++++++++++++------ 3 files changed, 32 insertions(+), 18 deletions(-) diff --git a/src/freedreno/ci/freedreno-a618-fails.txt b/src/freedreno/ci/freedreno-a618-fails.txt index b279b89beeb..3c924ff61d5 100644 --- a/src/freedreno/ci/freedreno-a618-fails.txt +++ b/src/freedreno/ci/freedreno-a618-fails.txt @@ -17,7 +17,3 @@ spill-dEQP-VK.graphicsfuzz.cov-function-two-loops-limit-using-arguments-array-el # Fails when TU_DEBUG=forcebin is set gmem-dEQP-VK.spirv_assembly.instruction.graphics.variable_pointers.graphics.writes_two_buffers_geom,Fail gmem-dEQP-VK.spirv_assembly.instruction.graphics.variable_pointers.graphics.writes_two_buffers_vert,Fail - -# Looks like a segfault, not assertion failure -spill-dEQP-VK.graphicsfuzz.cov-nested-loops-float-bits-to-int-increment-array,Crash -spill-dEQP-VK.graphicsfuzz.cov-two-nested-loops-switch-case-matrix-array-increment,Crash diff --git a/src/freedreno/ci/freedreno-a630-fails.txt b/src/freedreno/ci/freedreno-a630-fails.txt index dd2f2ce38a6..2d279f4c424 100644 --- a/src/freedreno/ci/freedreno-a630-fails.txt +++ b/src/freedreno/ci/freedreno-a630-fails.txt @@ -50,10 +50,6 @@ spill-dEQP-VK.graphicsfuzz.cov-function-two-loops-limit-using-arguments-array-el gmem-dEQP-VK.spirv_assembly.instruction.graphics.variable_pointers.graphics.writes_two_buffers_geom,Fail gmem-dEQP-VK.spirv_assembly.instruction.graphics.variable_pointers.graphics.writes_two_buffers_vert,Fail -# Looks like a segfault, not assertion failure -spill-dEQP-VK.graphicsfuzz.cov-nested-loops-float-bits-to-int-increment-array,Crash -spill-dEQP-VK.graphicsfuzz.cov-two-nested-loops-switch-case-matrix-array-increment,Crash - glx@glx_arb_create_context_es2_profile@invalid opengl es version,Fail glx@glx_arb_create_context_no_error@no error,Fail glx@glx_arb_create_context_robustness@invalid reset notification strategy,Fail diff --git a/src/freedreno/ir3/ir3_spill.c b/src/freedreno/ir3/ir3_spill.c index d28b1c7af9f..cc361b1ac66 100644 --- a/src/freedreno/ir3/ir3_spill.c +++ b/src/freedreno/ir3/ir3_spill.c @@ -1867,15 +1867,31 @@ simplify_phi_node(struct ir3_instruction *phi) return true; } +static struct ir3_register * +simplify_phi_def(struct ir3_register *def) +{ + if (def->instr->opc == OPC_META_PHI) { + struct ir3_instruction *phi = def->instr; + + /* Note: this function is always called at least once after visiting the + * phi, so either there has been a simplified phi in the meantime, in + * which case we will set progress=true and visit the definition again, or + * phi->data already has the most up-to-date value. Therefore we don't + * have to recursively check phi->data. + */ + if (phi->data) + return phi->data; + } + + return def; +} + static void simplify_phi_srcs(struct ir3_instruction *instr) { foreach_src (src, instr) { - if (src->def && src->def->instr->opc == OPC_META_PHI) { - struct ir3_instruction *phi = src->def->instr; - if (phi->data) - src->def = phi->data; - } + if (src->def) + src->def = simplify_phi_def(src->def); } } @@ -1905,6 +1921,10 @@ simplify_phi_nodes(struct ir3 *ir) simplify_phi_srcs(instr); } + /* Visit phi nodes in the sucessors to make sure that phi sources are + * always visited at least once after visiting the definition they + * point to. See note in simplify_phi_def() for why this is necessary. + */ for (unsigned i = 0; i < 2; i++) { struct ir3_block *succ = block->successors[i]; if (!succ) @@ -1912,11 +1932,13 @@ simplify_phi_nodes(struct ir3 *ir) foreach_instr (instr, &succ->instr_list) { if (instr->opc != OPC_META_PHI) break; - if (instr->flags & IR3_INSTR_UNUSED) - continue; - - simplify_phi_srcs(instr); - progress |= simplify_phi_node(instr); + if (instr->flags & IR3_INSTR_UNUSED) { + if (instr->data) + instr->data = simplify_phi_def(instr->data); + } else { + simplify_phi_srcs(instr); + progress |= simplify_phi_node(instr); + } } } }