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: Skip redundant imm to register writes #11094

Merged
merged 1 commit into from Sep 25, 2022

Conversation

Sintendo
Copy link
Member

@Sintendo Sintendo commented Sep 25, 2022

When a guest register is an immediate, it may be necessary to move this value into a register. This is handled by gpr.R(), which lacks context on how the register will be used. This leads to cases where the immediate is written to a register, only for it to be overwritten. Take for example this code generated by srwx:

0x5280031b   mov    w27, #0x18
0x53187edb   lsr    w27, w22, #24

gpr.BindToRegister() does have this context through the do_load parameter, but didn't handle immediates. By adding this logic, we can intelligently skip the write when do_load is false.

When a guest register is an immediate, it may be necessary to move this
value into a register. This is handled by gpr.R(), which lacks context
on how the register will be used. This leads to cases where the
immediate is written to a register, only for it to be overwritten. Take
for example this code generated by srwx:

0x5280031b   mov    w27, #0x18
0x53187edb   lsr    w27, w22, dolphin-emu#24

gpr.BindToRegister() does have this context through the do_load
parameter, but didn't handle immediates. By adding this logic, we can
intelligently skip the write when do_load is false.
@AdmiralCurtiss AdmiralCurtiss merged commit 9ca1c0f into dolphin-emu:master Sep 25, 2022
11 checks passed
@Sintendo Sintendo deleted the arm64bind2regimm branch September 26, 2022 06:25
@dvessel
Copy link
Contributor

dvessel commented Oct 1, 2022

F-Zero broken after this PR and Rogue Squadron won’t start. All I get is a black screen.

M1 Arm MacBook.

GFZE01_2022-10-01_09-28-51

@K0bin
Copy link
Contributor

K0bin commented Oct 1, 2022

FYI, I can reproduce the F-Zero issue on x86, so it's unlikely that this PR caused it.

@dvessel
Copy link
Contributor

dvessel commented Oct 1, 2022

I checked out the commit before this and it was fixed. Not sure what’s going on here.

@JosJuice
Copy link
Member

JosJuice commented Oct 1, 2022

This PR seems to be causing the issue in F-Zero GX for me. Android, AArch64, OpenGL.

@Sintendo
Copy link
Member Author

Sintendo commented Oct 1, 2022

I don't have access to this game myself, but perhaps unconditionally setting the dirty flag can fix this.

@JosJuice
Copy link
Member

JosJuice commented Oct 1, 2022

Unconditionally setting the dirty flag can mess things up in MMU games when do_load is false. I'll try to debug this.

@Sintendo
Copy link
Member Author

Sintendo commented Oct 1, 2022

Perhaps changing the condition to if (reg_type == RegType::Immediate && set_dirty) is safer, but I'll admit that I don't fully understand the semantics of set_dirty.

@JosJuice
Copy link
Member

JosJuice commented Oct 1, 2022

This PR uncovered an underlying bug in JitArm64 that I've now fixed in PR #11114.

Can't reproduce the problem on x86-64.

@K0bin
Copy link
Contributor

K0bin commented Oct 1, 2022

image

That's on x86.

@JosJuice
Copy link
Member

JosJuice commented Oct 1, 2022

I would advise bisecting.

@K0bin
Copy link
Contributor

K0bin commented Oct 1, 2022

My issue seems to be related to the x86 JIT. It's fixed with the cached interpreter and if I disable "JIT Floating Point".

@AdmiralCurtiss
Copy link
Contributor

Runs fine here:

image

You seem to be running a custom build, with that -dirty in the header, possibly related to one of your changes?

@K0bin
Copy link
Contributor

K0bin commented Oct 2, 2022

After some back and forth on IRC it turned out that my build was simply missing the game ini workaround for F-Zero.

@JosJuice
Copy link
Member

JosJuice commented Oct 2, 2022

The Rogue Squadron II issue was indeed related to the dirty flag. Fixed in PR #11117.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
5 participants