From 69b75fed12eae2d500da89d400d036739ff95b4c Mon Sep 17 00:00:00 2001 From: Martino Fontana Date: Mon, 22 Dec 2025 12:28:12 +0100 Subject: [PATCH] Jit: Followed unconditional branches are a property added by the Analyzer, not something to assume If an unconditional branch is not the last instruction of the block, assuming that this means that the branch must have been followed is an unsafe assumption on how the analyzer works that may not hold true in the future. As a bonus: JitArm64, unlike x86, didn't skip followed `bcx`s. Now it does. --- .../CachedInterpreter/CachedInterpreter.cpp | 2 +- Source/Core/Core/PowerPC/Jit64/Jit_Branch.cpp | 25 ++++----- .../Core/Core/PowerPC/Jit64/Jit_Integer.cpp | 2 +- .../Core/PowerPC/JitArm64/JitArm64_Branch.cpp | 56 +++++++++++++++---- Source/Core/Core/PowerPC/PPCAnalyst.cpp | 34 +++++------ Source/Core/Core/PowerPC/PPCAnalyst.h | 15 ++++- 6 files changed, 88 insertions(+), 46 deletions(-) diff --git a/Source/Core/Core/PowerPC/CachedInterpreter/CachedInterpreter.cpp b/Source/Core/Core/PowerPC/CachedInterpreter/CachedInterpreter.cpp index 2ec59b1180..5717ba122b 100644 --- a/Source/Core/Core/PowerPC/CachedInterpreter/CachedInterpreter.cpp +++ b/Source/Core/Core/PowerPC/CachedInterpreter/CachedInterpreter.cpp @@ -412,7 +412,7 @@ bool CachedInterpreter::DoJit(u32 em_address, JitBlock* b, u32 nextPC) operands); } - if (op.branchIsIdleLoop) + if (js.op->branchAction == PPCAnalyst::BranchAction::IdleLoop) Write(CheckIdle, {m_system.GetCoreTiming(), js.blockStart}); if (op.canEndBlock) WriteEndBlock(); diff --git a/Source/Core/Core/PowerPC/Jit64/Jit_Branch.cpp b/Source/Core/Core/PowerPC/Jit64/Jit_Branch.cpp index 4667238720..9975aca5e8 100644 --- a/Source/Core/Core/PowerPC/Jit64/Jit_Branch.cpp +++ b/Source/Core/Core/PowerPC/Jit64/Jit_Branch.cpp @@ -110,15 +110,14 @@ void Jit64::bx(UGeckoInstruction inst) INSTRUCTION_START JITDISABLE(bJITBranchOff); - // We must always process the following sentence - // even if the blocks are merged by PPCAnalyst::Flatten(). + // We must always process the following sentence, even if branch following is inlining the + // function call. if (inst.LK) MOV(32, PPCSTATE_LR, Imm32(js.compilerPC + 4)); - // If this is not the last instruction of a block, - // we will skip the rest process. - // Because PPCAnalyst::Flatten() merged the blocks. - if (!js.isLastInstruction) + // PPCAnalyzer::Analyze() followed the branch, thus the next instruction of the block is the + // destination of the branch, no need to do anything. + if (js.op->branchAction == PPCAnalyst::BranchAction::Follow) { WriteBranchWatch(js.compilerPC, js.op->branchTo, inst, CallerSavedRegistersInUse()); if (inst.LK && !js.op->skipLRStack) @@ -139,7 +138,7 @@ void Jit64::bx(UGeckoInstruction inst) if (inst.LK) AND(32, PPCSTATE(cr), Imm32(~(0xFF000000))); #endif - if (js.op->branchIsIdleLoop) + if (js.op->branchAction == PPCAnalyst::BranchAction::IdleLoop) { WriteIdleExit(js.op->branchTo); } @@ -179,11 +178,9 @@ void Jit64::bcx(UGeckoInstruction inst) if (inst.LK) MOV(32, PPCSTATE_LR, Imm32(js.compilerPC + 4)); - // If this is not the last instruction of a block - // and an unconditional branch, we will skip the rest process. - // Because PPCAnalyst::Flatten() merged the blocks. - if (!js.isLastInstruction && (inst.BO & BO_DONT_DECREMENT_FLAG) && - (inst.BO & BO_DONT_CHECK_CONDITION)) + // PPCAnalyzer::Analyze() followed this unconditional branch, thus the next instruction of the + // block is the destination of the branch, no need to do anything. + if (js.op->branchAction == PPCAnalyst::BranchAction::Follow) { WriteBranchWatch(js.compilerPC, js.op->branchTo, inst, CallerSavedRegistersInUse()); if (inst.LK && !js.op->skipLRStack) @@ -203,7 +200,7 @@ void Jit64::bcx(UGeckoInstruction inst) fpr.Flush(); WriteBranchWatch(js.compilerPC, js.op->branchTo, inst, {}); - if (js.op->branchIsIdleLoop) + if (js.op->branchAction == PPCAnalyst::BranchAction::IdleLoop) { WriteIdleExit(js.op->branchTo); } @@ -332,7 +329,7 @@ void Jit64::bclrx(UGeckoInstruction inst) gpr.Flush(); fpr.Flush(); - if (js.op->branchIsIdleLoop) + if (js.op->branchAction == PPCAnalyst::BranchAction::IdleLoop) { WriteBranchWatch(js.compilerPC, js.op->branchTo, inst, {}); WriteIdleExit(js.op->branchTo); diff --git a/Source/Core/Core/PowerPC/Jit64/Jit_Integer.cpp b/Source/Core/Core/PowerPC/Jit64/Jit_Integer.cpp index c77b831229..bc26e088a6 100644 --- a/Source/Core/Core/PowerPC/Jit64/Jit_Integer.cpp +++ b/Source/Core/Core/PowerPC/Jit64/Jit_Integer.cpp @@ -375,7 +375,7 @@ void Jit64::DoMergedBranch() const UGeckoInstruction& next = js.op[1].inst; const u32 nextPC = js.op[1].address; - if (js.op[1].branchIsIdleLoop) + if (js.op[1].branchAction == PPCAnalyst::BranchAction::IdleLoop) { if (next.LK) MOV(32, PPCSTATE_SPR(SPR_LR), Imm32(nextPC + 4)); diff --git a/Source/Core/Core/PowerPC/JitArm64/JitArm64_Branch.cpp b/Source/Core/Core/PowerPC/JitArm64/JitArm64_Branch.cpp index 8f391538c5..d5406b2355 100644 --- a/Source/Core/Core/PowerPC/JitArm64/JitArm64_Branch.cpp +++ b/Source/Core/Core/PowerPC/JitArm64/JitArm64_Branch.cpp @@ -119,7 +119,9 @@ void JitArm64::bx(UGeckoInstruction inst) STR(IndexType::Unsigned, WA, PPC_REG, PPCSTATE_OFF_SPR(SPR_LR)); } - if (!js.isLastInstruction) + // PPCAnalyzer::Analyze() followed the branch, thus the next instruction of the block is the + // destination of the branch, no need to do anything. + if (js.op->branchAction == PPCAnalyst::BranchAction::Follow) { if (IsBranchWatchEnabled()) { @@ -143,7 +145,7 @@ void JitArm64::bx(UGeckoInstruction inst) gpr.Flush(FlushMode::Full, ARM64Reg::INVALID_REG); fpr.Flush(FlushMode::Full, ARM64Reg::INVALID_REG); - if (js.op->branchIsIdleLoop) + if (js.op->branchAction == PPCAnalyst::BranchAction::IdleLoop) { if (IsBranchWatchEnabled()) { @@ -178,10 +180,19 @@ void JitArm64::bcx(UGeckoInstruction inst) INSTRUCTION_START JITDISABLE(bJITBranchOff); - auto WA = gpr.GetScopedReg(); - // If WA isn't needed for WriteExit, it can be safely clobbered. - auto WB = (inst.LK && !js.op->branchIsIdleLoop) ? gpr.GetScopedReg() : - Arm64GPRCache::ScopedARM64Reg(WA.GetReg()); + Arm64GPRCache::ScopedARM64Reg WA = ARM64Reg::INVALID_REG; + if ((inst.BO & BO_DONT_DECREMENT_FLAG) == 0 || inst.LK || + js.op->branchAction != PPCAnalyst::BranchAction::Follow) + { + WA = gpr.GetScopedReg(); + } + // If WA isn't needed for WriteExit, it can be safely clobbered. Same if the function returns + // before WB is used in the first place (if the branch is followed). + // While it may seem desirable to declare it later, note that GetScopedReg() may flush a register, + // an operation that cannot be skipped: thus, it must declared before emitting a branch. + auto WB = (inst.LK && js.op->branchAction == PPCAnalyst::BranchAction::Normal) ? + gpr.GetScopedReg() : + Arm64GPRCache::ScopedARM64Reg(WA.GetReg()); { FixupBranch pCTRDontBranch; @@ -211,6 +222,28 @@ void JitArm64::bcx(UGeckoInstruction inst) STR(IndexType::Unsigned, WA, PPC_REG, PPCSTATE_OFF_SPR(SPR_LR)); } + // PPCAnalyzer::Analyze() followed this unconditional branch, thus the next instruction of the + // block is the destination of the branch, no need to do anything. + if (js.op->branchAction == PPCAnalyst::BranchAction::Follow) + { + if (IsBranchWatchEnabled()) + { + BitSet32 gpr_caller_save = gpr.GetCallerSavedUsed(); + if (WA != ARM64Reg::INVALID_REG && js.op->skipLRStack) + gpr_caller_save[DecodeReg(WA)] = false; + WriteBranchWatch(js.compilerPC, js.op->branchTo, inst, gpr_caller_save, + fpr.GetCallerSavedUsed()); + } + if (inst.LK && !js.op->skipLRStack) + { + // We have to fake the stack as the RET instruction was not + // found in the same block. This is a big overhead, but still + // better than calling the dispatcher. + FakeLKExit(js.compilerPC + 4, WA); + } + return; + } + gpr.Flush(FlushMode::MaintainState, WB); fpr.Flush(FlushMode::MaintainState, ARM64Reg::INVALID_REG); @@ -220,7 +253,7 @@ void JitArm64::bcx(UGeckoInstruction inst) WriteBranchWatch(js.compilerPC, js.op->branchTo, inst, gpr_caller_save, fpr.GetCallerSavedUsed()); } - if (js.op->branchIsIdleLoop) + if (js.op->branchAction == PPCAnalyst::BranchAction::IdleLoop) { // make idle loops go faster ARM64Reg XA = EncodeRegTo64(WA); @@ -355,19 +388,20 @@ void JitArm64::bclrx(UGeckoInstruction inst) if (conditional) { gpr_caller_save = gpr.GetCallerSavedUsed(); - if (js.op->branchIsIdleLoop) + if (js.op->branchAction == PPCAnalyst::BranchAction::IdleLoop) gpr_caller_save[DecodeReg(WA)] = false; fpr_caller_save = fpr.GetCallerSavedUsed(); } else { - gpr_caller_save = - js.op->branchIsIdleLoop ? BitSet32{} : BitSet32{DecodeReg(WA)} & CALLER_SAVED_GPRS; + gpr_caller_save = js.op->branchAction == PPCAnalyst::BranchAction::IdleLoop ? + BitSet32{} : + BitSet32{DecodeReg(WA)} & CALLER_SAVED_GPRS; fpr_caller_save = {}; } WriteBranchWatchDestInRegister(js.compilerPC, WA, inst, gpr_caller_save, fpr_caller_save); } - if (js.op->branchIsIdleLoop) + if (js.op->branchAction == PPCAnalyst::BranchAction::IdleLoop) { // make idle loops go faster ARM64Reg XA = EncodeRegTo64(WA); diff --git a/Source/Core/Core/PowerPC/PPCAnalyst.cpp b/Source/Core/Core/PowerPC/PPCAnalyst.cpp index 386e349e3b..8f522e5dfa 100644 --- a/Source/Core/Core/PowerPC/PPCAnalyst.cpp +++ b/Source/Core/Core/PowerPC/PPCAnalyst.cpp @@ -702,7 +702,6 @@ void PPCAnalyzer::SetInstructionStats(CodeBlock* block, CodeOp* code, if (opinfo->flags & FL_IN_FLOAT_S) code->fregsIn[code->inst.FS] = true; - code->branchUsesCtr = false; code->branchTo = UINT32_MAX; // For branch with immediate addresses (bx/bcx), compute the destination. @@ -719,21 +718,20 @@ void PPCAnalyzer::SetInstructionStats(CodeBlock* block, CodeOp* code, code->branchTo = SignExt16(code->inst.BD << 2); else code->branchTo = code->address + SignExt16(code->inst.BD << 2); - if (!(code->inst.BO & BO_DONT_DECREMENT_FLAG)) - code->branchUsesCtr = true; - } - else if (code->inst.OPCD == 19 && code->inst.SUBOP10 == 16) // bclrx - { - if (!(code->inst.BO & BO_DONT_DECREMENT_FLAG)) - code->branchUsesCtr = true; - } - else if (code->inst.OPCD == 19 && code->inst.SUBOP10 == 528) // bcctrx - { - if (!(code->inst.BO & BO_DONT_DECREMENT_FLAG)) - code->branchUsesCtr = true; } } +static bool DoesBranchUseCtr(CodeOp* code) +{ + if (code->inst.OPCD == 16 || // bcx + (code->inst.OPCD == 19 && code->inst.SUBOP10 == 16) || // bclrx + (code->inst.OPCD == 19 && code->inst.SUBOP10 == 528) // bcctrx + ) + return !(code->inst.BO & BO_DONT_DECREMENT_FLAG); + else + return false; +} + bool PPCAnalyzer::IsBusyWaitLoop(CodeBlock* block, CodeOp* code, size_t instructions) const { // Very basic algorithm to detect busy wait loops: @@ -752,7 +750,7 @@ bool PPCAnalyzer::IsBusyWaitLoop(CodeBlock* block, CodeOp* code, size_t instruct { if (code[i].opinfo->type == OpType::Branch) { - if (code[i].branchUsesCtr) + if (DoesBranchUseCtr(&code[i])) return false; if (code[i].branchTo == block->m_address && i == instructions) return true; @@ -946,12 +944,14 @@ u32 PPCAnalyzer::Analyze(u32 address, CodeBlock* block, CodeBuffer* buffer, } } - code[i].branchIsIdleLoop = - code[i].branchTo == block->m_address && IsBusyWaitLoop(block, code, i); + if (code[i].branchTo == block->m_address && IsBusyWaitLoop(block, code, i)) + code[i].branchAction = BranchAction::IdleLoop; - if (follow && numFollows < BRANCH_FOLLOWING_THRESHOLD) + if (follow && code[i].branchAction != BranchAction::IdleLoop && + numFollows < BRANCH_FOLLOWING_THRESHOLD) { // Follow the unconditional branch. + code[i].branchAction = BranchAction::Follow; numFollows++; address = code[i].branchTo; } diff --git a/Source/Core/Core/PowerPC/PPCAnalyst.h b/Source/Core/Core/PowerPC/PPCAnalyst.h index f9081bae93..b152baf0a7 100644 --- a/Source/Core/Core/PowerPC/PPCAnalyst.h +++ b/Source/Core/Core/PowerPC/PPCAnalyst.h @@ -26,6 +26,18 @@ class CPUThreadGuard; namespace PPCAnalyst { +enum class BranchAction : u8 +{ + Normal, + // The analyzer followed the branch, and thus the next instruction corresponds to its target. This + // can only be done if the branch is unconditional. + Follow, + // The branch represents a loop with idempotent properties: if the condition is true, then the + // emulation should skip directly to timing handling, as interrupts are the only thing that can + // change the loop's effects. + IdleLoop, +}; + struct CodeOp // 16B { UGeckoInstruction inst; @@ -38,8 +50,7 @@ struct CodeOp // 16B s8 fregOut = 0; BitSet8 crIn; BitSet8 crOut; - bool branchUsesCtr = false; - bool branchIsIdleLoop = false; + BranchAction branchAction = {}; BitSet8 wantsCR; bool wantsFPRF = false; bool wantsCA = false;