From de41fd00a5094ee06e4742ce74ef5dd4b991149e Mon Sep 17 00:00:00 2001 From: Alyssa Rosenzweig Date: Thu, 21 Jul 2022 18:15:56 -0400 Subject: [PATCH] pan/bi: Don't read nonexistant destinations Codebase audit. In preparation to dynamically allocate destinations. Signed-off-by: Alyssa Rosenzweig Part-of: --- src/panfrost/bifrost/bi_lower_swizzle.c | 5 +++-- src/panfrost/bifrost/bi_opt_constant_fold.c | 1 + src/panfrost/bifrost/bi_opt_copy_prop.c | 3 ++- src/panfrost/bifrost/bi_opt_message_preload.c | 2 +- src/panfrost/bifrost/bi_opt_mod_props.c | 8 ++++--- src/panfrost/bifrost/bi_pack.c | 6 +++--- src/panfrost/bifrost/bi_schedule.c | 30 ++++++++++++++++++--------- src/panfrost/bifrost/bi_scoreboard.c | 4 ++-- src/panfrost/bifrost/valhall/va_pack.c | 3 ++- 9 files changed, 39 insertions(+), 23 deletions(-) diff --git a/src/panfrost/bifrost/bi_lower_swizzle.c b/src/panfrost/bifrost/bi_lower_swizzle.c index 542ae35..6dc762a 100644 --- a/src/panfrost/bifrost/bi_lower_swizzle.c +++ b/src/panfrost/bifrost/bi_lower_swizzle.c @@ -251,7 +251,7 @@ bi_lower_swizzle(bi_context *ctx) BITSET_WORD *replicates_16 = calloc(sizeof(bi_index), ctx->ssa_alloc); bi_foreach_instr_global(ctx, ins) { - if (bi_is_ssa(ins->dest[0]) && bi_instr_replicates(ins, replicates_16)) + if (ins->nr_dests && bi_is_ssa(ins->dest[0]) && bi_instr_replicates(ins, replicates_16)) BITSET_SET(replicates_16, ins->dest[0].value); if (ins->op == BI_OPCODE_SWZ_V2I16 && bi_is_ssa(ins->src[0]) && @@ -264,7 +264,8 @@ bi_lower_swizzle(bi_context *ctx) * Valhall, we will want to optimize this. For now, default * to Bifrost compatible behaviour. */ - ins->dest[0].swizzle = BI_SWIZZLE_H01; + if (ins->nr_dests) + ins->dest[0].swizzle = BI_SWIZZLE_H01; } free(replicates_16); diff --git a/src/panfrost/bifrost/bi_opt_constant_fold.c b/src/panfrost/bifrost/bi_opt_constant_fold.c index 0099385..bebf393 100644 --- a/src/panfrost/bifrost/bi_opt_constant_fold.c +++ b/src/panfrost/bifrost/bi_opt_constant_fold.c @@ -95,6 +95,7 @@ bi_opt_constant_fold(bi_context *ctx) if (unsupported) continue; /* Replace with constant move, to be copypropped */ + assert(ins->nr_dests == 1); bi_builder b = bi_init_builder(ctx, bi_after_instr(ins)); bi_mov_i32_to(&b, ins->dest[0], bi_imm_u32(replace)); bi_remove_instruction(ins); diff --git a/src/panfrost/bifrost/bi_opt_copy_prop.c b/src/panfrost/bifrost/bi_opt_copy_prop.c index 901b589..89798e5 100644 --- a/src/panfrost/bifrost/bi_opt_copy_prop.c +++ b/src/panfrost/bifrost/bi_opt_copy_prop.c @@ -77,7 +77,7 @@ bi_opt_copy_prop(bi_context *ctx) /* Lower the split to moves, copyprop cleans up */ bi_builder b = bi_init_builder(ctx, bi_before_instr(I)); - for (unsigned d = 0; d < I->nr_dests; ++d) + bi_foreach_dest(I, d) bi_mov_i32_to(&b, I->dest[d], collect->src[d]); bi_remove_instruction(I); @@ -101,6 +101,7 @@ bi_opt_copy_prop(bi_context *ctx) replace = chained; } + assert(ins->nr_dests == 1); replacement[ins->dest[0].value] = replace; } diff --git a/src/panfrost/bifrost/bi_opt_message_preload.c b/src/panfrost/bifrost/bi_opt_message_preload.c index 3a26101..87d7008 100644 --- a/src/panfrost/bifrost/bi_opt_message_preload.c +++ b/src/panfrost/bifrost/bi_opt_message_preload.c @@ -96,7 +96,7 @@ bi_opt_message_preload(bi_context *ctx) bi_builder b = bi_init_builder(ctx, bi_before_nonempty_block(block)); bi_foreach_instr_in_block_safe(block, I) { - if (!bi_is_ssa(I->dest[0])) continue; + if (I->nr_dests != 1 || !bi_is_ssa(I->dest[0])) continue; struct bifrost_message_preload msg; diff --git a/src/panfrost/bifrost/bi_opt_mod_props.c b/src/panfrost/bifrost/bi_opt_mod_props.c index f4fc470..fff9332 100644 --- a/src/panfrost/bifrost/bi_opt_mod_props.c +++ b/src/panfrost/bifrost/bi_opt_mod_props.c @@ -192,8 +192,10 @@ bi_opt_mod_prop_forward(bi_context *ctx) bi_instr **lut = calloc(sizeof(bi_instr *), ctx->ssa_alloc); bi_foreach_instr_global_safe(ctx, I) { - if (bi_is_ssa(I->dest[0])) - lut[I->dest[0].value] = I; + bi_foreach_dest(I, d) { + if (bi_is_ssa(I->dest[d])) + lut[I->dest[d].value] = I; + } bi_foreach_src(I, s) { if (!bi_is_ssa(I->src[s])) @@ -391,7 +393,7 @@ bi_opt_mod_prop_backward(bi_context *ctx) } } - if (!bi_is_ssa(I->dest[0])) + if (!I->nr_dests || !bi_is_ssa(I->dest[0])) continue; bi_instr *use = uses[I->dest[0].value]; diff --git a/src/panfrost/bifrost/bi_pack.c b/src/panfrost/bifrost/bi_pack.c index 4aa2012..ff42a08 100644 --- a/src/panfrost/bifrost/bi_pack.c +++ b/src/panfrost/bifrost/bi_pack.c @@ -137,7 +137,7 @@ bi_assign_slots(bi_tuple *now, bi_tuple *prev) * +ATEST wants its destination written to both a staging register * _and_ a regular write, because it may not generate a message */ - if (prev->add && (!write_dreg || prev->add->op == BI_OPCODE_ATEST)) { + if (prev->add && prev->add->nr_dests && (!write_dreg || prev->add->op == BI_OPCODE_ATEST)) { bi_index idx = prev->add->dest[0]; if (idx.type == BI_INDEX_REGISTER) { @@ -146,8 +146,8 @@ bi_assign_slots(bi_tuple *now, bi_tuple *prev) } } - if (prev->fma) { - bi_index idx = (prev->fma)->dest[0]; + if (prev->fma && prev->fma->nr_dests) { + bi_index idx = prev->fma->dest[0]; if (idx.type == BI_INDEX_REGISTER) { if (now->regs.slot23.slot3) { diff --git a/src/panfrost/bifrost/bi_schedule.c b/src/panfrost/bifrost/bi_schedule.c index c0e46be..3f02860 100644 --- a/src/panfrost/bifrost/bi_schedule.c +++ b/src/panfrost/bifrost/bi_schedule.c @@ -585,8 +585,7 @@ bi_can_add(bi_instr *ins) static bool bi_must_not_last(bi_instr *ins) { - return !bi_is_null(ins->dest[0]) && !bi_is_null(ins->dest[1]) && - (ins->op != BI_OPCODE_TEXC_DUAL); + return (ins->nr_dests >= 2) && (ins->op != BI_OPCODE_TEXC_DUAL); } /* Check for a message-passing instruction. +DISCARD.f32 is special-cased; we @@ -970,6 +969,9 @@ bi_has_staging_passthrough_hazard(bi_index fma, bi_instr *add) static bool bi_has_cross_passthrough_hazard(bi_tuple *succ, bi_instr *ins) { + if (ins->nr_dests == 0) + return false; + bi_foreach_instr_in_tuple(succ, pins) { bi_foreach_src(pins, s) { if (bi_is_word_equiv(ins->dest[0], pins->src[s]) && @@ -1103,7 +1105,7 @@ bi_instr_schedulable(bi_instr *instr, /* If this choice of FMA would force a staging passthrough, the ADD * instruction must support such a passthrough */ - if (tuple->add && bi_has_staging_passthrough_hazard(instr->dest[0], tuple->add)) + if (tuple->add && instr->nr_dests && bi_has_staging_passthrough_hazard(instr->dest[0], tuple->add)) return false; /* If this choice of destination would force a cross-tuple passthrough, the next tuple must support that */ @@ -1138,9 +1140,15 @@ bi_instr_schedulable(bi_instr *instr, return false; /* Count effective reads for the successor */ - unsigned succ_reads = bi_count_succ_reads(instr->dest[0], - tuple->add ? tuple->add->dest[0] : bi_null(), - tuple->prev_reads, tuple->nr_prev_reads); + unsigned succ_reads = 0; + + if (instr->nr_dests) { + bool has_t1 = tuple->add && tuple->add->nr_dests; + succ_reads = bi_count_succ_reads(instr->dest[0], + has_t1 ? tuple->add->dest[0] : bi_null(), + tuple->prev_reads, + tuple->nr_prev_reads); + } /* Successor must satisfy R+W <= 4, so we require W <= 4-R */ if ((signed) total_writes > (4 - (signed) succ_reads)) @@ -1310,9 +1318,11 @@ bi_use_passthrough(bi_instr *ins, bi_index old, bool except_sr) { /* Optional for convenience */ - if (!ins || bi_is_null(old)) + if (!ins) return; + assert(!bi_is_null(old)); + bi_foreach_src(ins, i) { if ((i == 0 || i == 4) && except_sr) continue; @@ -1339,12 +1349,12 @@ bi_rewrite_passthrough(bi_tuple prec, bi_tuple succ) { bool sr_read = succ.add ? bi_opcode_props[succ.add->op].sr_read : false; - if (prec.add) { + if (prec.add && prec.add->nr_dests) { bi_use_passthrough(succ.fma, prec.add->dest[0], BIFROST_SRC_PASS_ADD, false); bi_use_passthrough(succ.add, prec.add->dest[0], BIFROST_SRC_PASS_ADD, sr_read); } - if (prec.fma) { + if (prec.fma && prec.fma->nr_dests) { bi_use_passthrough(succ.fma, prec.fma->dest[0], BIFROST_SRC_PASS_FMA, false); bi_use_passthrough(succ.add, prec.fma->dest[0], BIFROST_SRC_PASS_FMA, sr_read); } @@ -1776,7 +1786,7 @@ bi_schedule_clause(bi_context *ctx, bi_block *block, struct bi_worklist st, uint * passthrough the sources of the ADD that read from the * destination of the FMA */ - if (tuple->fma) { + if (tuple->fma && tuple->fma->nr_dests) { bi_use_passthrough(tuple->add, tuple->fma->dest[0], BIFROST_SRC_STAGE, false); } diff --git a/src/panfrost/bifrost/bi_scoreboard.c b/src/panfrost/bifrost/bi_scoreboard.c index f54e413..b103040 100644 --- a/src/panfrost/bifrost/bi_scoreboard.c +++ b/src/panfrost/bifrost/bi_scoreboard.c @@ -153,8 +153,8 @@ bi_write_mask(bi_instr *I) * Obscurely, ATOM_CX is sr_write but can ignore the staging register in * certain circumstances; this does not require consideration. */ - if (bi_opcode_props[I->op].sr_write && bi_is_null(I->dest[0]) && - !bi_is_null(I->src[0])) { + if (bi_opcode_props[I->op].sr_write && I->nr_dests && + bi_is_null(I->dest[0]) && !bi_is_null(I->src[0])) { unsigned reg = I->src[0].value; unsigned count = bi_count_write_registers(I, 0); diff --git a/src/panfrost/bifrost/valhall/va_pack.c b/src/panfrost/bifrost/valhall/va_pack.c index 2b419a7..7b45845 100644 --- a/src/panfrost/bifrost/valhall/va_pack.c +++ b/src/panfrost/bifrost/valhall/va_pack.c @@ -191,6 +191,7 @@ va_pack_atom_opc_1(const bi_instr *I) static unsigned va_pack_dest(const bi_instr *I) { + assert(I->nr_dests); return va_pack_reg(I, I->dest[0]) | (va_pack_wrmask(I) << 6); } @@ -442,7 +443,7 @@ va_pack_alu(const bi_instr *I) if (info.has_dest && info.nr_staging_dests == 0) { hex |= (uint64_t) va_pack_dest(I) << 40; } else if (info.nr_staging_dests == 0 && info.nr_staging_srcs == 0) { - pack_assert(I, bi_is_null(I->dest[0])); + pack_assert(I, I->nr_dests == 0); hex |= 0xC0ull << 40; /* Placeholder */ } -- 2.7.4