From 901872e2d985d63c7e463ada7576051bfc085258 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 | 53 +++++++++++++++---- Source/Core/Core/PowerPC/PPCAnalyst.cpp | 34 ++++++------ Source/Core/Core/PowerPC/PPCAnalyst.h | 15 +++++- 6 files changed, 85 insertions(+), 46 deletions(-) diff --git a/Source/Core/Core/PowerPC/CachedInterpreter/CachedInterpreter.cpp b/Source/Core/Core/PowerPC/CachedInterpreter/CachedInterpreter.cpp index 3ec24a3016..6eb2f2ada8 100644 --- a/Source/Core/Core/PowerPC/CachedInterpreter/CachedInterpreter.cpp +++ b/Source/Core/Core/PowerPC/CachedInterpreter/CachedInterpreter.cpp @@ -406,7 +406,7 @@ bool CachedInterpreter::DoJit(u32 em_address, JitBlock* b, u32 nextPC) operands); } - if (op.branchIsIdleLoop) + if (js.op->branchKind == PPCAnalyst::BranchKind::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..a99a6b89f6 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 next instruction of the block to the destination of the + // branch, no need to do anything. + if (js.op->branchKind == PPCAnalyst::BranchKind::Followed) { 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->branchKind == PPCAnalyst::BranchKind::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() found that this branch is unconditional and thus followed the next + // instruction of the block to the destination of the branch, no need to do anything. + if (js.op->branchKind == PPCAnalyst::BranchKind::Followed) { 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->branchKind == PPCAnalyst::BranchKind::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->branchKind == PPCAnalyst::BranchKind::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 f3c329cef4..be86ad6a70 100644 --- a/Source/Core/Core/PowerPC/Jit64/Jit_Integer.cpp +++ b/Source/Core/Core/PowerPC/Jit64/Jit_Integer.cpp @@ -381,7 +381,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].branchKind == PPCAnalyst::BranchKind::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 191c423876..10802157e7 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 next instruction of the block to the destination of the + // branch, no need to do anything. + if (js.op->branchKind == PPCAnalyst::BranchKind::Followed) { if (IsBranchWatchEnabled()) { @@ -143,7 +145,7 @@ void JitArm64::bx(UGeckoInstruction inst) gpr.Flush(FlushMode::All, ARM64Reg::INVALID_REG); fpr.Flush(FlushMode::All, ARM64Reg::INVALID_REG); - if (js.op->branchIsIdleLoop) + if (js.op->branchKind == PPCAnalyst::BranchKind::IdleLoop) { if (IsBranchWatchEnabled()) { @@ -178,10 +180,16 @@ 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 (js.op->branchKind != PPCAnalyst::BranchKind::Followed || inst.LK) + 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->branchKind == PPCAnalyst::BranchKind::Normal) ? + gpr.GetScopedReg() : + Arm64GPRCache::ScopedARM64Reg(WA.GetReg()); { FixupBranch pCTRDontBranch; @@ -211,6 +219,28 @@ void JitArm64::bcx(UGeckoInstruction inst) STR(IndexType::Unsigned, WA, PPC_REG, PPCSTATE_OFF_SPR(SPR_LR)); } + // PPCAnalyzer::Analyze() found that this branch is unconditional and thus followed the next + // instruction of the block to the destination of the branch, no need to do anything. + if (js.op->branchKind == PPCAnalyst::BranchKind::Followed) + { + 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 +250,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->branchKind == PPCAnalyst::BranchKind::IdleLoop) { // make idle loops go faster ARM64Reg XA = EncodeRegTo64(WA); @@ -355,19 +385,20 @@ void JitArm64::bclrx(UGeckoInstruction inst) if (conditional) { gpr_caller_save = gpr.GetCallerSavedUsed(); - if (js.op->branchIsIdleLoop) + if (js.op->branchKind == PPCAnalyst::BranchKind::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->branchKind == PPCAnalyst::BranchKind::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->branchKind == PPCAnalyst::BranchKind::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 8c35ad4923..c1866424d9 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; @@ -939,12 +937,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].branchKind = BranchKind::IdleLoop; - if (follow && numFollows < BRANCH_FOLLOWING_THRESHOLD) + if (follow && code[i].branchKind != BranchKind::IdleLoop && + numFollows < BRANCH_FOLLOWING_THRESHOLD) { // Follow the unconditional branch. + code[i].branchKind = BranchKind::Followed; numFollows++; address = code[i].branchTo; } diff --git a/Source/Core/Core/PowerPC/PPCAnalyst.h b/Source/Core/Core/PowerPC/PPCAnalyst.h index ed0242498b..da2b61e200 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 BranchKind +{ + 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. + Followed, + // The branch represent 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; + BranchKind branchKind = {}; BitSet8 wantsCR; bool wantsFPRF = false; bool wantsCA = false;