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

JitRegCache: Refactor register cache #7492

Merged
merged 84 commits into from Nov 9, 2018

Conversation

8 participants
@MerryMage
Member

MerryMage commented Oct 13, 2018

Similar idea to #3565, different implementation.

  • Automatic unlocking using RAII.
  • Forking of regcache state with Fork.
  • "Revertable bindings" / "Transactions": A bind can be declared with RevertableBind, and a future Revert/Commit would rollback or complete the transaction.
  • Unlike the previous proposal, no laziness. Realization occurs with an explicit Realize call.
  • I have tried to make as direct a translation as possible here for ease of verifying correctness and will leave futher cleanup to future PRs.
  • I have done the port in so that this PR should be bisectable for debugging if necessary; each intermediate commit should compile and work properly.
  • Please note that this is incomplete.

This refactor allows for easier development of future optimizations.

TODO:

  • Complete transition.
  • Remove old regcache interface.
  • Allow Scratch to allocate arbitrary scratch registers.
  • Make GetFreeXReg, IsFreeX, NumFreeRegisters private.
  • Remove Lock, LockX, UnlockX, UnlockAll, UnlockAllX, R, RX, KillImmediate, BindToRegister, StoreFromRegister, FlushLockX, DiscardRegContentsIfCached from interface.
  • Remove mode argument from Flush; all flushes should be full flushes. Use Fork instead if maintaining state is required.

We introduce two new concepts, RCX64Reg and RCOpArg, which are analogues of X64Reg and OpArg.

As an example, here is what an addition of register a and register b to be stored in register d would look like:

RCX64Reg Rd = gpr.Bind(d, RCMode::Write);
RCOpArg Ra = gpr.Use(a, RCMode::Read);
RCOpArg Rb = gpr.Use(b, RCMode::Read);
RegAlloc::Realize(Rd, Ra, Rb);

MOV_sum(32, Rd, Ra, Rb);

Each declaration is a constraint, and when Realize is called these constraints are realized. Note that there is no need to check for d == a || d == b &c&c, this is automatically handled by constraints.

There are several constraint types:

  • Use: Can be memory, bound, or immediate
  • UseNoImm: Can be memory or bound. Intended to replace old uses of KillImmediate.
  • BindOrImm: Can be bound or immediate. I found this pattern in multiple places in the JIT and found this helpful.
  • Bind: Must be bound to a register. Intended to replace old uses of BindToRegiser.
  • RevertableBind: Mentioned above, replaces js.revertGprLoad/js.revertFprLoad.

One can use RCOpArg with immediates, which allows for cleaner code. RCOpArg::Imm32 constructs such an immediate.

return std::visit([](auto&& arg) { return std::variant<To...>{arg}; }, v);
}
};
} // namespcae detail

This comment has been minimized.

@lioncash
}
}
bool IsCompatible(RCMode mode, ConstraintLoc loc, bool should_revertable)

This comment has been minimized.

@lioncash

lioncash Oct 13, 2018

Member

shouldn't this be a const member function?

@BhaaLseN

Wondering if all those members for RCOpArg, RCX64Reg, RCForkGuard etc. should have the m_ prefix or not...

Show resolved Hide resolved Source/Core/Core/PowerPC/Jit64/RegCache/JitRegCache.cpp Outdated
gpr.BindToRegister(a, true, true);
RCX64Reg scratch_guard = gpr.Scratch(RSCRATCH_EXTRA);
RCOpArg Ra = update ? gpr.Bind(a, RCMode::ReadWrite) : gpr.Use(a, RCMode::Read);
RCOpArg Rb = indexed ? gpr.Use(b, RCMode::Read) : RCOpArg::Imm32((u32)offset);

This comment has been minimized.

@BhaaLseN

BhaaLseN Oct 13, 2018

Member

static_cast<u32>(offset)

fpr.BindToRegister(s, false, true);
RCX64Reg scratch_guard = gpr.Scratch(RSCRATCH_EXTRA);
RCX64Reg Ra = gpr.Bind(a, update ? RCMode::ReadWrite : RCMode::Read);
RCOpArg Rb = indexed ? gpr.Use(b, RCMode::Read) : RCOpArg::Imm32((u32)offset);

This comment has been minimized.

@BhaaLseN

BhaaLseN Oct 13, 2018

Member

static_cast<u32>(offset)

@MerryMage

This comment has been minimized.

Member

MerryMage commented Oct 14, 2018

std::variant usage changed from overloaded{} to std::get_if due to lack of support on android/osx builders.

@MerryMage MerryMage changed the title from [RFC/WIP] JitRegCache: Refactor register cache to JitRegCache: Refactor register cache Oct 15, 2018

@MerryMage

This comment has been minimized.

Member

MerryMage commented Oct 15, 2018

Completed port.

@MerryMage

This comment has been minimized.

Member

MerryMage commented Oct 17, 2018

  • Fix bug in mffsx.
  • Add additional BitSet operators (will rename commit on rebase).
@delroth

This comment has been minimized.

Member

delroth commented Nov 9, 2018

The new API is great, the refactoring commits I spot checked look fine, let's just merge this to get some mileage and give @JMC47 some more work :P

@delroth delroth merged commit 61b9ef3 into dolphin-emu:master Nov 9, 2018

10 checks passed

default Very basic checks passed, handed off to Buildbot.
Details
lint Build succeeded on builder lint
Details
pr-android Build succeeded on builder pr-android
Details
pr-deb-dbg-x64 Build succeeded on builder pr-deb-dbg-x64
Details
pr-deb-x64 Build succeeded on builder pr-deb-x64
Details
pr-freebsd-x64 Build succeeded on builder pr-freebsd-x64
Details
pr-osx-x64 Build succeeded on builder pr-osx-x64
Details
pr-ubu-x64 Build succeeded on builder pr-ubu-x64
Details
pr-win-dbg-x64 Build succeeded on builder pr-win-dbg-x64
Details
pr-win-x64 Build succeeded on builder pr-win-x64
Details
@JMC47

This comment has been minimized.

Contributor

JMC47 commented Nov 9, 2018

@hthh

This comment has been minimized.

hthh commented Nov 9, 2018

Just look out for regressions in any games that exercise the JIT in tricky ways, I'd say.

It looks like this broke Rogue Squadron II (doesn't boot). I bisected to the Jit_LoadStore: lXXx commit 534db3b (thanks for making it bisectable!) My guess is a RevertableBind issue? I haven't looked further than that yet.

@nbohr1more

This comment has been minimized.

nbohr1more commented Nov 9, 2018

I'm unable to start Spiderman 2 and Rouge Squadron 3 after this.

Looks like Gamecube games are the most affected?

@JosJuice

This comment has been minimized.

Contributor

JosJuice commented Nov 9, 2018

Rather, it seems like MMU games are the most affected. Those are all examples of MMU games.

@nbohr1more

This comment has been minimized.

nbohr1more commented Nov 9, 2018

Whoa! Good find.

Commenting out MMU = 1 from Spiderman 2 makes it perfectly playable.

I'm guessing this wasn't supposed to be a replacement for MMU though...

Edit: Rogue Leader doesn't start with this same workaroud :(

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment