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: Add temp reg parameter to Arm64RegCache::Flush #9892

Merged
merged 1 commit into from Jul 22, 2021

Conversation

JosJuice
Copy link
Member

We currently have a bug when calling Arm64GPRCache::Flush with FlushMode::MaintainState, zero free host registers, and at least one guest register containing an immediate. We end up grabbing a temporary register from the register cache in order to be able to write the immediate to memory, but grabbing a temporary register when there are zero free registers causes the least recently used register to be flushed in a way which does not maintain the state of the register cache.

To get around this, require callers to pass in a temporary register in the GPR MaintainState case. In other cases, passing in a temporary register is not required but can help avoid spilling a register (if the caller already had a temporary register at hand anyway, which in particular will be the case in my upcoming memcheck pull request).

@ghost
Copy link

ghost commented Jul 18, 2021

This will improve android performance?

@JosJuice
Copy link
Member Author

No, but it fixes a bug.

We currently have a bug when calling Arm64GPRCache::Flush with
FlushMode::MaintainState, zero free host registers, and at least
one guest register containing an immediate. We end up grabbing
a temporary register from the register cache in order to be
able to write the immediate to memory, but grabbing a temporary
register when there are zero free registers causes the least
recently used register to be flushed in a way which does not
maintain the state of the register cache.

To get around this, require callers to pass in a temporary
register in the GPR MaintainState case. In other cases,
passing in a temporary register is not required but can help
avoid spilling a register (if the caller already had a
temporary register at hand anyway, which in particular will
be the case in my upcoming memcheck pull request).
@degasus
Copy link
Member

degasus commented Jul 22, 2021

Honestly and at least one guest register containing an immediate is a bug in the old implementation. The issue only happens when all host registers are allocated, so you have host register you can flush first and immediates later. Not the less, it might be good to have a tmp reg in the flush call, however I'm not so sure about having one in the flush_all one.

@JosJuice
Copy link
Member Author

The issue only happens when all host registers are allocated, so you have host register you can flush first and immediates later.

But flushing registers with FlushMode::MaintainState does not actually free up the corresponding host register for use, so changing the order in which we flush does nothing to fix the bug.

Not the less, it might be good to have a tmp reg in the flush call, however I'm not so sure about having one in the flush_all one.

Yes, in the FlushMode::All case there's no real benefit to it.

@degasus degasus merged commit 674e2aa into dolphin-emu:master Jul 22, 2021
11 checks passed
@JosJuice JosJuice deleted the jitarm64-flush-temp branch July 22, 2021 08:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
2 participants