From c98ddc778a34683e55d29122edba53ac4969deea Mon Sep 17 00:00:00 2001 From: "Juan A. Suarez Romero" Date: Wed, 8 Sep 2021 10:04:45 +0200 Subject: [PATCH] broadcom/compiler: force a last thrsw for spilling As we don't know if we are going to have spilling or not, emit always a last thrsw at the end of the shader. If later we don't have spillings and we don't need that last thrsw, we remove it and switch back to the previous one. This way we ensure all the spilling happens always before the last thrsw. v2 (Juan): - Rework the code to force a last thrsw and remove later if no spilling v3: - Merge functionality inside vir_emit_last_thrsw (Iago) - Add vir_restore_last_thrsw (Juan) v4 (Iago): - Fix/add new comments - Rename variables/parameters v5 (Iago): - Fix comments - Add assertion Cc: mesa-stable Fixes: https://gitlab.freedesktop.org/mesa/mesa/-/issues/4760 Reviewed-by: Iago Toral Quiroga Signed-off-by: Juan A. Suarez Romero Part-of: --- src/broadcom/compiler/nir_to_vir.c | 60 ++++++++++++++++++- src/broadcom/compiler/vir_register_allocate.c | 20 +------ 2 files changed, 59 insertions(+), 21 deletions(-) diff --git a/src/broadcom/compiler/nir_to_vir.c b/src/broadcom/compiler/nir_to_vir.c index 439a8ab342f..d0a89f1a7d4 100644 --- a/src/broadcom/compiler/nir_to_vir.c +++ b/src/broadcom/compiler/nir_to_vir.c @@ -3913,9 +3913,25 @@ vir_remove_thrsw(struct v3d_compile *c) c->last_thrsw = NULL; } +/** + * This makes sure we have a top-level last thread switch which signals the + * start of the last thread section, which may include adding a new thrsw + * instruction if needed. We don't allow spilling in the last thread section, so + * if we need to do any spills that inject additional thread switches later on, + * we ensure this thread switch will still be the last thread switch in the + * program, which makes last thread switch signalling a lot easier when we have + * spilling. If in the end we don't need to spill to compile the program and we + * injected a new thread switch instruction here only for that, we will + * eventually restore the previous last thread switch and remove the one we + * added here. + */ static void -vir_emit_last_thrsw(struct v3d_compile *c) +vir_emit_last_thrsw(struct v3d_compile *c, + struct qinst **restore_last_thrsw, + bool *restore_scoreboard_lock) { + *restore_last_thrsw = c->last_thrsw; + /* On V3D before 4.1, we need a TMU op to be outstanding when thread * switching, so disable threads if we didn't do any TMU ops (each of * which would have emitted a THRSW). @@ -3924,7 +3940,7 @@ vir_emit_last_thrsw(struct v3d_compile *c) c->threads = 1; if (c->last_thrsw) vir_remove_thrsw(c); - return; + *restore_last_thrsw = NULL; } /* If we're threaded and the last THRSW was in conditional code, then @@ -3947,8 +3963,34 @@ vir_emit_last_thrsw(struct v3d_compile *c) vir_emit_thrsw(c); } + /* If we have not inserted a last thread switch yet, do it now to ensure + * any potential spilling we do happens before this. If we don't spill + * in the end, we will restore the previous one. + */ + if (*restore_last_thrsw == c->last_thrsw) { + if (*restore_last_thrsw) + (*restore_last_thrsw)->is_last_thrsw = false; + *restore_scoreboard_lock = c->lock_scoreboard_on_first_thrsw; + vir_emit_thrsw(c); + } else { + *restore_last_thrsw = c->last_thrsw; + } + + assert(c->last_thrsw); + c->last_thrsw->is_last_thrsw = true; +} + +static void +vir_restore_last_thrsw(struct v3d_compile *c, + struct qinst *thrsw, + bool scoreboard_lock) +{ + assert(c->last_thrsw); + vir_remove_instruction(c, c->last_thrsw); + c->last_thrsw = thrsw; if (c->last_thrsw) c->last_thrsw->is_last_thrsw = true; + c->lock_scoreboard_on_first_thrsw = scoreboard_lock; } /* There's a flag in the shader for "center W is needed for reasons other than @@ -3986,8 +4028,14 @@ v3d_nir_to_vir(struct v3d_compile *c) nir_to_vir(c); + bool restore_scoreboard_lock = false; + struct qinst *restore_last_thrsw; + /* Emit the last THRSW before STVPM and TLB writes. */ - vir_emit_last_thrsw(c); + vir_emit_last_thrsw(c, + &restore_last_thrsw, + &restore_scoreboard_lock); + switch (c->s->info.stage) { case MESA_SHADER_FRAGMENT: @@ -4086,6 +4134,12 @@ v3d_nir_to_vir(struct v3d_compile *c) vir_remove_thrsw(c); } + /* If we didn't spill, then remove the last thread switch we injected + * artificially (if any) and restore the previous one. + */ + if (!c->spills && c->last_thrsw != restore_last_thrsw) + vir_restore_last_thrsw(c, restore_last_thrsw, restore_scoreboard_lock); + if (c->spills && (V3D_DEBUG & (V3D_DEBUG_VIR | v3d_debug_flag_for_shader_stage(c->s->info.stage)))) { diff --git a/src/broadcom/compiler/vir_register_allocate.c b/src/broadcom/compiler/vir_register_allocate.c index ddb9957b8ef..08698b4ece1 100644 --- a/src/broadcom/compiler/vir_register_allocate.c +++ b/src/broadcom/compiler/vir_register_allocate.c @@ -261,7 +261,7 @@ v3d_spill_reg(struct v3d_compile *c, int spill_temp) } struct qinst *last_thrsw = c->last_thrsw; - assert(!last_thrsw || last_thrsw->is_last_thrsw); + assert(last_thrsw && last_thrsw->is_last_thrsw); int start_num_temps = c->num_temps; @@ -347,29 +347,13 @@ v3d_spill_reg(struct v3d_compile *c, int spill_temp) spill_offset); } } - - /* If we didn't have a last-thrsw inserted by nir_to_vir and - * we've been inserting thrsws, then insert a new last_thrsw - * right before we start the vpm/tlb sequence for the last - * thread segment. - */ - if (!is_uniform && !last_thrsw && c->last_thrsw && - (v3d_qpu_writes_vpm(&inst->qpu) || - v3d_qpu_uses_tlb(&inst->qpu))) { - c->cursor = vir_before_inst(inst); - vir_emit_thrsw(c); - - last_thrsw = c->last_thrsw; - last_thrsw->is_last_thrsw = true; - } } } /* Make sure c->last_thrsw is the actual last thrsw, not just one we * inserted in our most recent unspill. */ - if (last_thrsw) - c->last_thrsw = last_thrsw; + c->last_thrsw = last_thrsw; /* Don't allow spilling of our spilling instructions. There's no way * they can help get things colored.