diff --git a/src/amd/compiler/aco_insert_NOPs.cpp b/src/amd/compiler/aco_insert_NOPs.cpp index 1283d141552..fe9f826d2b9 100644 --- a/src/amd/compiler/aco_insert_NOPs.cpp +++ b/src/amd/compiler/aco_insert_NOPs.cpp @@ -273,6 +273,8 @@ struct NOP_ctx_gfx11 { std::bitset sgpr_read_by_valu; /* SGPR pairs, excluding null, exec, m0 and scc */ std::bitset sgpr_read_by_valu_then_wr_by_valu; RegCounterMap<11> sgpr_read_by_valu_then_wr_by_salu; + /* Force emitting a wait mitigating VALUReadSGPRHazard before the next ALU instruction. */ + bool force_valu_read_sgpr_wait = false; void join(const NOP_ctx_gfx11& other) { @@ -293,6 +295,7 @@ struct NOP_ctx_gfx11 { sgpr_read_by_valu |= other.sgpr_read_by_valu; sgpr_read_by_valu_then_wr_by_valu |= other.sgpr_read_by_valu_then_wr_by_valu; sgpr_read_by_valu_then_wr_by_salu.join_min(other.sgpr_read_by_valu_then_wr_by_salu); + force_valu_read_sgpr_wait |= other.force_valu_read_sgpr_wait; } bool operator==(const NOP_ctx_gfx11& other) const @@ -312,7 +315,8 @@ struct NOP_ctx_gfx11 { other.sgpr_read_by_valu_as_lanemask_then_wr_by_valu && vgpr_written_by_wmma == other.vgpr_written_by_wmma && sgpr_read_by_valu == other.sgpr_read_by_valu && - sgpr_read_by_valu_then_wr_by_salu == other.sgpr_read_by_valu_then_wr_by_salu; + sgpr_read_by_valu_then_wr_by_salu == other.sgpr_read_by_valu_then_wr_by_salu && + force_valu_read_sgpr_wait == other.force_valu_read_sgpr_wait; } }; @@ -1604,6 +1608,12 @@ handle_instruction_gfx11(State& state, NOP_ctx_gfx11& ctx, aco_ptr& unsigned expiry_count = instr->isSALU() ? 10 : 11; uint16_t imm = 0xffff; + if (ctx.force_valu_read_sgpr_wait) { + imm &= 0xfffe; + wait.sa_sdst = 0; + ctx.force_valu_read_sgpr_wait = false; + } + for (Operand& op : instr->operands) { if (op.physReg() >= m0) continue; @@ -2050,6 +2060,15 @@ insert_NOPs(Program* program) * SGPR might have been read by VALU if there was a previous shader part. */ initial_ctx.sgpr_read_by_valu.flip(); + /* We cannot assume the s_setpc source has not been read by VALU in the preceding shader/ + * shader part, and there are GPU hangs in the wild suggesting that the s_setpc source may + * be susceptible to VALUReadSGPRHazard. It is impossible for the previous part to mitigate + * this, and it is not always known which register the s_setpc source was in, so force a + * wait to be emitted at the start of this part. + * + * TODO: This hypothesis is not yet conclusively proven. More testing is needed. + */ + initial_ctx.force_valu_read_sgpr_wait = true; } mitigate_hazards(program, diff --git a/src/amd/compiler/tests/test_insert_nops.cpp b/src/amd/compiler/tests/test_insert_nops.cpp index d4ce6ec43ec..0efc1043fc6 100644 --- a/src/amd/compiler/tests/test_insert_nops.cpp +++ b/src/amd/compiler/tests/test_insert_nops.cpp @@ -1764,8 +1764,12 @@ BEGIN_TEST(insert_nops.valu_read_sgpr.previous_part) /* Raytracing shaders have a prolog and may also be split into several parts. */ program->stage = raytracing_cs; - /* Despite the SGPR never being read by a VALU in this shader, a sa_sdst(0) is needed. */ + /* Despite the SGPR never being read by a VALU in this shader, a sa_sdst(0) is needed. + * The first instruction is also a sa_sdst(0) in RT shaders to protect against reads of the + * setpc target. + */ //>> p_unit_test 0 + //! s_waitcnt_depctr sa_sdst(0) //! s1: %0:s[4] = s_mov_b32 0 //! s_waitcnt_depctr sa_sdst(0) //! s1: %0:s[64] = s_mov_b32 %0:s[4]