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.

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();

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<true>(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<true>(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<true>(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<true>(js.compilerPC, js.op->branchTo, inst, {});
       WriteIdleExit(js.op->branchTo);

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));

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<true>(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<true>(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);

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;
     }

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;