dolphin icon indicating copy to clipboard operation
dolphin copied to clipboard

JitArm64: Never check downcount on block entry

Open JosJuice opened this issue 2 years ago • 7 comments

Jumping between linked blocks currently works as follows: First, at the end of the first block, we check if the downcount is greater than zero. If it is, we jump to the normalEntry of the block. So far so good. But if the downcount wasn't greater than zero, we jump to the checkedEntry of the block, which checks the downcount again and then jumps to do_timing if it's less than zero (which seems like an off by one error – Jit64 doesn't do anything like this). This second check is rather redundant. Let's jump to do_timing where we previously jumped to checkedEntry.

Jit64 doesn't check the downcount on block entry. See PR #7634.

JosJuice avatar Oct 22 '22 13:10 JosJuice

Console error & backtrace when starting Rogue Leader. Error happens at startup.

54:20:151 Core/PowerPC/JitArm64/Jit.cpp:440 E[MASTER]: Warning: An error occurred.

  Condition: GetCodePtr() == farcode_2_addr
  File: /Users/foo/dolphin/Source/Core/Core/PowerPC/JitArm64/Jit.cpp
  Line: 440
  Function: WriteExit

Ignore and continue?
bt
Process 47151 stopped
* thread #30, name = 'CPU-GPU thread', stop reason = EXC_BREAKPOINT (code=1, subcode=0x100baa868)
    frame #0: 0x0000000100baa868 Dolphin`JitArm64::WriteExit(unsigned int, bool, unsigned int) + 776
Dolphin`JitArm64::WriteExit:
->  0x100baa868 <+776>: brk    #0x1
    0x100baa86c <+780>: b      0x100baa870               ; <+784>
    0x100baa870 <+784>: b      0x100baa874               ; <+788>
    0x100baa874 <+788>: add    x0, x19, #0x1b0
Target 0: (Dolphin) stopped.
(lldb) bt
* thread #30, name = 'CPU-GPU thread', stop reason = EXC_BREAKPOINT (code=1, subcode=0x100baa868)
  * frame #0: 0x0000000100baa868 Dolphin`JitArm64::WriteExit(unsigned int, bool, unsigned int) + 776
    frame #1: 0x0000000100bc240c Dolphin`JitArm64::bcx(UGeckoInstruction) + 652
    frame #2: 0x0000000100bf2b14 Dolphin`JitArm64::CompileInstruction(PPCAnalyst::CodeOp&) + 104
    frame #3: 0x0000000100ba5ac0 Dolphin`JitArm64::DoJit(unsigned int, JitBlock*, unsigned int) + 2596
    frame #4: 0x0000000100ba4750 Dolphin`JitArm64::Jit(unsigned int, bool) + 764
    frame #5: 0x0000000100ba3ee4 Dolphin`JitArm64::Jit(unsigned int) + 40
    frame #6: 0x0000000100aece64 Dolphin`JitTrampoline(JitBase&, unsigned int) + 40
    frame #7: 0x0000000170d510b4

dvessel avatar Dec 01 '22 14:12 dvessel

Thanks! Fixed.

JosJuice avatar Dec 03 '22 09:12 JosJuice

I brought this up on discord. Posting it here so it doesn’t get lost.

RS2 freezes when switching to the targeting computer (Y Button). Usually doesn’t freeze but this is from a dtm file that can catch it every time. I can share it if needed.

https://youtu.be/Oq__l07W93I?t=17

And the stack trace

dvessel avatar Jan 13 '23 12:01 dvessel

@dvessel Can you check if this patch fixes it?

diff --git a/Source/Core/Core/PowerPC/JitArm64/JitArm64Cache.cpp b/Source/Core/Core/PowerPC/JitArm64/JitArm64Cache.cpp
index 512f211037..18a6f34c39 100644
--- a/Source/Core/Core/PowerPC/JitArm64/JitArm64Cache.cpp
+++ b/Source/Core/Core/PowerPC/JitArm64/JitArm64Cache.cpp
@@ -71,7 +71,11 @@ void JitArm64BlockCache::WriteLinkBlock(Arm64Gen::ARM64XEmitter& emit,
   // Use a fixed number of instructions so we have enough room for any patching needed later.
   const u8* end = start + BLOCK_LINK_SIZE;
   while (emit.GetCodePtr() < end)
+  {
     emit.BRK(101);
+    if (emit.HasWriteFailed())
+      return;
+  }
   ASSERT(emit.GetCodePtr() == end);
 }

The reason why I didn't push this patch to the pull request is because if I were to push without rebasing, the buildbot would get upset, and if I were to rebase, your DTM might not sync anymore. If you can confirm that this fixes it, I'll rebase and push.

JosJuice avatar Feb 19 '23 21:02 JosJuice

Rebased the PR and still freezes. Applied patch and now getting this error at the same point where it freezes:

The dtm file doesn’t depend on a save state so rebasing is fine.

12:01:858 Core/PowerPC/JitArm64/Jit.cpp:476 E[MASTER]: Warning: An error occurred.

  Condition: GetCodePtr() == farcode_1_addr
  File: dolphin/Source/Core/Core/PowerPC/JitArm64/Jit.cpp
  Line: 476
  Function: WriteExit

Ignore and continue?

Process 29811 stopped
* thread #33, name = 'CPU-GPU thread', stop reason = EXC_BREAKPOINT (code=1, subcode=0x100be42ec)
    frame #0: 0x0000000100be42ec Dolphin`JitArm64::WriteExit(unsigned int, bool, unsigned int) + 544
Dolphin`JitArm64::WriteExit:
->  0x100be42ec <+544>: brk    #0x1
    0x100be42f0 <+548>: b      0x100be42f4               ; <+552>
    0x100be42f4 <+552>: b      0x100be42f8               ; <+556>
    0x100be42f8 <+556>: add    x0, x19, #0x1b0
Target 0: (Dolphin) stopped.

(lldb) bt
* thread #33, name = 'CPU-GPU thread', stop reason = EXC_BREAKPOINT (code=1, subcode=0x100be42ec)
  * frame #0: 0x0000000100be42ec Dolphin`JitArm64::WriteExit(unsigned int, bool, unsigned int) + 544
    frame #1: 0x0000000100bfcb14 Dolphin`JitArm64::bcx(UGeckoInstruction) + 652
    frame #2: 0x0000000100c3070c Dolphin`JitArm64::CompileInstruction(PPCAnalyst::CodeOp&) + 104
    frame #3: 0x0000000100bdf298 Dolphin`JitArm64::DoJit(unsigned int, JitBlock*, unsigned int) + 2964
    frame #4: 0x0000000100bddc80 Dolphin`JitArm64::Jit(unsigned int, bool) + 848
    frame #5: 0x0000000100bdca88 Dolphin`JitArm64::Jit(unsigned int) + 40
    frame #6: 0x0000000100b1f9dc Dolphin`JitTrampoline(JitBase&, unsigned int) + 40
    frame #7: 0x00000002953e50c0

dvessel avatar Feb 19 '23 22:02 dvessel

The dtm file doesn’t depend on a save state so rebasing is fine.

My concern wasn't about savestates not loading, it was about the inputs desyncing. But okay, if it syncs for you after rebasing, I suppose we haven't had any sync-breaking changes since you made the DTM.

I've rebased the PR and added a patch that should fix the assert you reported getting. Does it also fix the freeze, or is that still happening? If it's still happening, could you bisect within the PR?

JosJuice avatar Feb 25 '23 11:02 JosJuice

It works! Thanks for the fix.

dvessel avatar Feb 25 '23 13:02 dvessel