From a4b164b57b7f3727a7b6ee0feb6c1808c68febe6 Mon Sep 17 00:00:00 2001 From: Iago Toral Quiroga Date: Mon, 21 Feb 2022 14:09:33 +0100 Subject: [PATCH] broadcom/compiler: only patch temps that existed before the current spill MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit When we spill we add new temps. We should be careful not to access liveness for these until we have re-computed it after all spills and fill for that the spilled temp have been processed so as to avoid out-of-bounds accesses to the c->temp_start and c->temp_end arrays. This fixes a crash in a Three.js demo when we try to patch register classes after a TMU spill that was caused because we would incorrectly try to patch the same temps we had just added for the spill itself, which is not only unnecessary but also incorrect since we these temps would not have liveness information available yet and thus would cause out of bounds accesses. Fixes: f3c3228522 ('broadcom/compiler: do not rebuild the interference graph after each spill') Reviewed-by: Alejandro Piñeiro Part-of: --- src/broadcom/compiler/v3d_compiler.h | 5 +++++ src/broadcom/compiler/vir_register_allocate.c | 9 +++++---- 2 files changed, 10 insertions(+), 4 deletions(-) diff --git a/src/broadcom/compiler/v3d_compiler.h b/src/broadcom/compiler/v3d_compiler.h index 58411bfdbc3..0d221c9d947 100644 --- a/src/broadcom/compiler/v3d_compiler.h +++ b/src/broadcom/compiler/v3d_compiler.h @@ -845,6 +845,11 @@ struct v3d_compile { struct qreg undef; uint32_t num_temps; + /* Number of temps in the program right before we spill a new temp. We + * use this to know which temps existed before a spill and which were + * added with the spill itself. + */ + uint32_t spill_start_num_temps; struct vir_cursor cursor; struct list_head blocks; diff --git a/src/broadcom/compiler/vir_register_allocate.c b/src/broadcom/compiler/vir_register_allocate.c index a970c634ace..6336d19686e 100644 --- a/src/broadcom/compiler/vir_register_allocate.c +++ b/src/broadcom/compiler/vir_register_allocate.c @@ -356,7 +356,7 @@ v3d_emit_spill_tmua(struct v3d_compile *c, * is not affected (and only the spilled temp starts at ip). Something * that ends at ip won't be affected either. */ - for (int i = 0; i < c->num_temps; i++) { + for (int i = 0; i < c->spill_start_num_temps; i++) { bool thrsw_cross = fill_dst ? c->temp_start[i] < ip && c->temp_end[i] >= ip : c->temp_start[i] < ip && c->temp_end[i] > ip; @@ -414,7 +414,7 @@ interferes(int32_t t0_start, int32_t t0_end, int32_t t1_start, int32_t t1_end) static void v3d_spill_reg(struct v3d_compile *c, int *acc_nodes, int spill_temp) { - int start_num_temps = c->num_temps; + c->spill_start_num_temps = c->num_temps; c->spilling = true; bool is_uniform = vir_is_mov_uniform(c, spill_temp); @@ -564,7 +564,7 @@ v3d_spill_reg(struct v3d_compile *c, int *acc_nodes, int spill_temp) /* Don't allow spilling of our spilling instructions. There's no way * they can help get things colored. */ - for (int i = start_num_temps; i < c->num_temps; i++) + for (int i = c->spill_start_num_temps; i < c->num_temps; i++) BITSET_CLEAR(c->spillable, i); /* Reset interference for spilled node */ @@ -592,7 +592,8 @@ v3d_spill_reg(struct v3d_compile *c, int *acc_nodes, int spill_temp) uint32_t node = c->ra_map.temp[i].node; c->ra_map.node[node].priority = c->temp_end[i] - c->temp_start[i]; - for (uint32_t j = MAX2(i + 1, start_num_temps); j < c->num_temps; j++) { + for (uint32_t j = MAX2(i + 1, c->spill_start_num_temps); + j < c->num_temps; j++) { if (interferes(c->temp_start[i], c->temp_end[i], c->temp_start[j], c->temp_end[j])) { ra_add_node_interference(c->g,