Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

JitArm64: Never check downcount on block entry #11191

Merged
merged 3 commits into from Aug 26, 2023

Conversation

JosJuice
Copy link
Member

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.

@dvessel
Copy link
Contributor

dvessel commented Dec 1, 2022

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

@JosJuice
Copy link
Member Author

JosJuice commented Dec 3, 2022

Thanks! Fixed.

dvessel added a commit to dvessel/dolphin that referenced this pull request Jan 6, 2023
@dvessel
Copy link
Contributor

dvessel commented Jan 13, 2023

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

@JosJuice
Copy link
Member Author

@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.

@dvessel
Copy link
Contributor

dvessel commented Feb 19, 2023

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

@JosJuice
Copy link
Member Author

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?

@dvessel
Copy link
Contributor

dvessel commented Feb 25, 2023

It works! Thanks for the fix.

@JosJuice JosJuice force-pushed the jitarm64-no-checked-entry branch 2 times, most recently from 9e302e7 to 00cede9 Compare February 25, 2023 13:37
dvessel added a commit to dvessel/dolphin that referenced this pull request Mar 3, 2023
Copy link
Contributor

@krnlyng krnlyng left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've added some minor comments. Feel free to completely ignore me though.

Source/Core/Core/PowerPC/JitArm64/Jit.cpp Outdated Show resolved Hide resolved
Source/Core/Core/PowerPC/JitArm64/Jit.cpp Outdated Show resolved Hide resolved
Source/Core/Core/PowerPC/JitArm64/Jit.cpp Show resolved Hide resolved
Source/Core/Core/PowerPC/JitArm64/Jit.cpp Outdated Show resolved Hide resolved
Source/Core/Core/PowerPC/JitArm64/Jit.cpp Show resolved Hide resolved
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 5236dc3.
It's now always identical to normalEntry.
Now block link nearcode is back to a length of three instructions.

Unfortunately, the code I'm adding to Jit.cpp ends up being a bit messy
because we need to handle the case of already being in farcode...
@JosJuice JosJuice merged commit cd31da9 into dolphin-emu:master Aug 26, 2023
11 checks passed
@JosJuice JosJuice deleted the jitarm64-no-checked-entry branch August 26, 2023 15:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
3 participants