From b2f475251ed9fa8b37cb98372e64d6166d48a089 Mon Sep 17 00:00:00 2001 From: Alyssa Rosenzweig Date: Fri, 31 Jul 2020 10:43:55 -0400 Subject: [PATCH] pan/mdg: Test for SSA before chasing addresses It's possible an SSA value depends on a register; in this case, chasing the source would result in a crash as the chase helper in NIR asserts is_ssa. Instead we should check a priori that all the argments are in fact SSA, bailing otherwise. In the piglit shader exhibiting this bug (by looping over the index), bailing on the ishl instruction is -necessary-. This is not merely us being cowardly to avoid seeing through the registers; indeed, if we wrote away the ishl instruction, the shift itself would have to be stored in a load/store register (r26/r27) which would preclude reading it in the loop, creating a register allocation failure later in the compile. So this is the correct solution due to the restricted semantics. Closes #3286 Signed-off-by: Alyssa Rosenzweig Reported-by: Icecream95 Fixes: f5401cb8866 ("pan/midgard: Add address analysis framework") Part-of: --- src/panfrost/midgard/midgard_address.c | 27 ++++++++++++++++++++++++-- 1 file changed, 25 insertions(+), 2 deletions(-) diff --git a/src/panfrost/midgard/midgard_address.c b/src/panfrost/midgard/midgard_address.c index c0d17775517..5e287dad14f 100644 --- a/src/panfrost/midgard/midgard_address.c +++ b/src/panfrost/midgard/midgard_address.c @@ -45,6 +45,20 @@ struct mir_address { unsigned bias; }; +static bool +mir_args_ssa(nir_ssa_scalar s, unsigned count) +{ + nir_alu_instr *alu = nir_instr_as_alu(s.def->parent_instr); + assert(count <= nir_op_infos[alu->op].num_inputs); + + for (unsigned i = 0; i < count; ++i) { + if (!alu->src[i].src.is_ssa) + return false; + } + + return true; +} + /* Matches a constant in either slot and moves it to the bias */ static void @@ -69,6 +83,9 @@ mir_match_iadd(struct mir_address *address, bool first_free) if (!address->B.def || !nir_ssa_scalar_is_alu(address->B)) return; + if (!mir_args_ssa(address->B, 2)) + return; + nir_op op = nir_ssa_scalar_alu_op(address->B); if (op != nir_op_iadd) return; @@ -96,6 +113,9 @@ mir_match_u2u64(struct mir_address *address) if (!address->B.def || !nir_ssa_scalar_is_alu(address->B)) return; + if (!mir_args_ssa(address->B, 1)) + return; + nir_op op = nir_ssa_scalar_alu_op(address->B); if (op != nir_op_u2u64) return; nir_ssa_scalar arg = nir_ssa_scalar_chase_alu_src(address->B, 0); @@ -112,6 +132,9 @@ mir_match_ishl(struct mir_address *address) if (!address->B.def || !nir_ssa_scalar_is_alu(address->B)) return; + if (!mir_args_ssa(address->B, 2)) + return; + nir_op op = nir_ssa_scalar_alu_op(address->B); if (op != nir_op_ishl) return; nir_ssa_scalar op1 = nir_ssa_scalar_chase_alu_src(address->B, 0); @@ -134,14 +157,14 @@ mir_match_mov(struct mir_address *address) if (address->A.def && nir_ssa_scalar_is_alu(address->A)) { nir_op op = nir_ssa_scalar_alu_op(address->A); - if (op == nir_op_mov) + if (op == nir_op_mov && mir_args_ssa(address->A, 1)) address->A = nir_ssa_scalar_chase_alu_src(address->A, 0); } if (address->B.def && nir_ssa_scalar_is_alu(address->B)) { nir_op op = nir_ssa_scalar_alu_op(address->B); - if (op == nir_op_mov) + if (op == nir_op_mov && mir_args_ssa(address->B, 1)) address->B = nir_ssa_scalar_chase_alu_src(address->B, 0); } } -- 2.30.2