Jit: Load the memory register only when the msr bits have changed and do not use jumps to load it. #12048

krnlyng · 2023-07-14T22:16:43Z

Not sure if this is worth the time, but i made it so here we go.

Basically turns the membase switch into a single instruction (except when msr changes, then it's more).

JosJuice

I was looking into the same thing in PR #11958, but in the end I never actually took it to the conclusion you did in your last commit here. Good to see it implemented in Jit64 too.

JosJuice · 2023-07-15T06:06:56Z

Source/Core/Core/PowerPC/JitArm64/JitAsm.cpp

+    LDR(IndexType::Unsigned, ARM64Reg::W27, PPC_REG, PPCSTATE_OFF(msr));
+    TST(ARM64Reg::W27, LogicalImm(1 << (31 - 27), 32));
+    MOVP2R(MEM_REG, jo.fastmem_arena ? memory.GetLogicalBase() : memory.GetLogicalPageMappingsBase());
+    MOVP2R(ARM64Reg::X25,


Please put the TST after the MOVP2Rs (both here and in the other instances of code like this). Putting the TST right after the LDR means that in-order CPUs won't be able to run the MOVP2Rs until the load completes.

JosJuice · 2023-07-15T06:09:06Z

Source/Core/Core/PowerPC/Jit64/Jit_SystemRegisters.cpp

+    MOV(64, R(RMEM), ImmPtr(memory.GetLogicalBase()));
+    MOV(64, R(RSCRATCH2), ImmPtr(memory.GetPhysicalBase()));
+    CMOVcc(64, RMEM, R(RSCRATCH2), CC_Z);
+    MOV(64, PPCSTATE(mem_ptr), R(RMEM));


This code is repeated between here and Jit_Branch.cpp. Please split it out to a separate function, like you did with UpdateMembase.

JosJuice · 2023-07-15T06:11:03Z

Source/Core/Core/PowerPC/JitArm64/Jit.cpp

-                      "PC {:#018x}, access address {:#018x}, memory base {:#018x}, MSR.DR {}",
-                      ctx->CTX_PC, access_address, memory_base, m_ppc_state.msr.DR);
+                      "PC {:#018x}, access address {:#018x}, memory base {:#018x}, MSR.DR {}, memptr {:#018x}, pbase {:#018x}, lbase {:#018x}",
+                      ctx->CTX_PC, access_address, memory_base, m_ppc_state.msr.DR, (u64)m_ppc_state.mem_ptr, (u64)memory.GetPhysicalBase(), (u64)memory.GetLogicalBase());


We prefer C++ style casts (static_cast) over C-style casts.

Actually, there's something even better you can do here: You can use fmt::ptr. Example:

dolphin/Source/Core/Common/JitRegister.cpp

Line 108 in 7f40c6f

const auto entry = fmt::format("{} {:x} {}\n", fmt::ptr(base_address), code_size, symbol_name);

JosJuice · 2023-07-15T06:11:34Z

Source/Core/Core/PowerPC/JitArm64/Jit.h

@@ -311,6 +311,9 @@ class JitArm64 : public JitBase, public Arm64Gen::ARM64CodeBlock, public CommonA
  void BeginTimeProfile(JitBlock* b);
  void EndTimeProfile(JitBlock* b);

+  // RMEM


There is nothing called "RMEM" in JitArm64. I assume you mean MEM_REG. Though with that said, I'm not sure how much information this comment really imparts...

JosJuice · 2023-07-15T06:14:22Z

Source/Core/Core/PowerPC/JitArm64/JitAsm.cpp

+    ARM64Reg msr = ARM64Reg::W27;
+    LDR(IndexType::Unsigned, msr, PPC_REG, PPCSTATE_OFF(msr));
+
+    UpdateMembase();


Do we really need to do UpdateMembase each time we're in the dispatcher? If you instead update the value of MEM_REG every time the msr changes, you shouldn't need to update MEM_REG here, because MEM_REG is callee-saved and isn't used for anything else in the JIT, and so should never get overwritten.

(I'm not sure if Jit64's RMEM is callee-saved.)

Yeah you're right, we should only need it when going through enter_code right?
I tired that but it didn't work, i'll look into why eventually but for now i need to catch a plane :).

Right, you'll need it for enter_code, but other than that it should just be needed when msr is changed.

(I'm not sure if Jit64's RMEM is callee-saved.)

Jit64's RMEM is callee-saved.

Ok, now i've finally figured this out, i forgot that GlobalAdvance also checks exceptions and missed the load there. Now it works for both x86_64 and aarch64 to only load the memory register when needed.

I'm a bit puzzled why it worked on x86_64 without that, but i guess the fastmem implementation there might be a bit more forgiving.

JosJuice · 2023-07-15T06:17:06Z

Source/Core/Core/PowerPC/JitInterface.cpp

+#endif
+#ifdef _M_ARM_64
+    ppc_state.mem_ptr = m_jit->jo.fastmem_arena ? memory.GetLogicalBase() : memory.GetLogicalPageMappingsBase();
+#endif


You could probably just use the AArch64 variant of this on all CPUs. Jit64 shouldn't rely on RMEM containing the memory base when there is no fastmem_arena, since in that case there is no memory base.

JosJuice

Overall the code is starting to look pretty good, but the commit history isn't very clean. The following commits should be melded into the commits that are the cause of the problems they're fixing:

JitArm64/Jit64: some cleanups for the previous changes.
JitArm64/Jit64: Fix linter errors.
JitArm64/Jit64: Remove now unused variable.
JitArm64/Jit64: Don't update the mem_ptr in JitInterface if m_jit is …

Or, alternatively... The approach this PR is taking has changed a bit since the PR was started, and some of the earlier changes in the PR are effectively made obsolete by later changes in the PR, especially "JitArm64/Jit64: Some minor optimizations to how msr is compared and l…". Maybe you could just squash all the commits into one?

JosJuice · 2023-07-27T15:49:32Z

Source/Core/Core/PowerPC/JitArm64/JitAsm.cpp

-           jo.fastmem_arena ? memory.GetLogicalBase() : memory.GetLogicalPageMappingsBase());
-    SetJumpTarget(membaseend);
+    ARM64Reg msr = ARM64Reg::W27;
+    LDR(IndexType::Unsigned, msr, PPC_REG, PPCSTATE_OFF(msr));


This LDR can be moved back to where it was before this PR.

This has now been moved, but not to where it was before this PR. Was that done intentionally, for better codegen?

sorry, i moved it back

Source/Core/Core/PowerPC/JitArm64/Jit.cpp

Source/Core/Core/PowerPC/JitArm64/JitAsm.cpp

Source/Core/Core/PowerPC/Jit64/Jit.cpp

krnlyng · 2023-07-27T22:55:15Z

Overall the code is starting to look pretty good, but the commit history isn't very clean. The following commits should be melded into the commits that are the cause of the problems they're fixing:
* JitArm64/Jit64: some cleanups for the previous changes.

* JitArm64/Jit64: Fix linter errors.

* JitArm64/Jit64: Remove now unused variable.

* JitArm64/Jit64: Don't update the mem_ptr in JitInterface if m_jit is …
Or, alternatively... The approach this PR is taking has changed a bit since the PR was started, and some of the earlier changes in the PR are effectively made obsolete by later changes in the PR, especially "JitArm64/Jit64: Some minor optimizations to how msr is compared and l…". Maybe you could just squash all the commits into one?

Hi, sorry for that, i was once told it's easier to follow a PR when the changes are incremental and only squashed once the PR is ready to go since it should make seeing what actually changed in between pushes easier. I'll squash everything.

JosJuice · 2023-07-28T07:41:05Z

Well, yes and no. We like to have multiple commits when they're cleanly separated different steps, but fixes to minor problems that were introduced earlier in the PR are best amended in the commit they were introduced. That way, reading through the whole PR becomes easier, though it does come with the tradeoff of making it harder to track what has changed between different versions of a PR, like you say.

Source/Core/Core/PowerPC/JitArm64/JitAsm.cpp

…necessary.

JosJuice · 2023-07-28T13:02:15Z

Let's get this merged after the next beta comes out.

AdmiralCurtiss · 2023-08-18T23:47:59Z

@JosJuice Do you want to merge this now?

JosJuice reviewed Jul 15, 2023

View reviewed changes

krnlyng changed the title ~~WIP: some random optimizations.~~ Load the memory register only when the msr bits have changed and do not use jumps to load it. Jul 18, 2023

krnlyng changed the title ~~Load the memory register only when the msr bits have changed and do not use jumps to load it.~~ Jit: Load the memory register only when the msr bits have changed and do not use jumps to load it. Jul 18, 2023

krnlyng force-pushed the someothertest branch from d4a9102 to 51684ab Compare July 18, 2023 19:50

JosJuice reviewed Jul 27, 2023

View reviewed changes

Source/Core/Core/PowerPC/Jit64/Jit.cpp Outdated Show resolved Hide resolved

krnlyng force-pushed the someothertest branch from f59f89d to 95d429c Compare July 27, 2023 22:55

JosJuice reviewed Jul 28, 2023

View reviewed changes

Source/Core/Core/PowerPC/JitArm64/JitAsm.cpp Outdated Show resolved Hide resolved

JitArm64/Jit64: Load the memory register without jumps and only when …

8bfcd2d

…necessary.

krnlyng force-pushed the someothertest branch from 95d429c to 8bfcd2d Compare July 28, 2023 11:31

JosJuice merged commit ed78949 into dolphin-emu:master Aug 19, 2023
11 checks passed

JosJuice mentioned this pull request Aug 19, 2023

JitArm64: Dispatcher optimizations #11958

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Jit: Load the memory register only when the msr bits have changed and do not use jumps to load it. #12048

Jit: Load the memory register only when the msr bits have changed and do not use jumps to load it. #12048

krnlyng commented Jul 14, 2023

JosJuice left a comment •

edited

JosJuice Jul 15, 2023

JosJuice Jul 15, 2023

JosJuice Jul 15, 2023

JosJuice Jul 15, 2023

JosJuice Jul 15, 2023 •

edited

krnlyng Jul 15, 2023

JosJuice Jul 15, 2023

merryhime Jul 15, 2023 •

edited

krnlyng Jul 18, 2023

JosJuice Jul 15, 2023 •

edited

JosJuice left a comment

JosJuice Jul 27, 2023

JosJuice Jul 28, 2023

krnlyng Jul 28, 2023

krnlyng commented Jul 27, 2023

JosJuice commented Jul 28, 2023

JosJuice commented Jul 28, 2023

AdmiralCurtiss commented Aug 18, 2023

Jit: Load the memory register only when the msr bits have changed and do not use jumps to load it. #12048

Jit: Load the memory register only when the msr bits have changed and do not use jumps to load it. #12048

Conversation

krnlyng commented Jul 14, 2023

JosJuice left a comment • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JosJuice Jul 15, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

merryhime Jul 15, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JosJuice Jul 15, 2023 • edited

Choose a reason for hiding this comment

JosJuice left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

krnlyng commented Jul 27, 2023

JosJuice commented Jul 28, 2023

JosJuice commented Jul 28, 2023

AdmiralCurtiss commented Aug 18, 2023

JosJuice left a comment •

edited

JosJuice Jul 15, 2023 •

edited

merryhime Jul 15, 2023 •

edited

JosJuice Jul 15, 2023 •

edited