JIT: Don't assume the reserved bits in GQRs are zero #736
Conversation
|
Note that Dirt only broke because of psq_st, but I figured I should modify psq_l for consistency, since if a game could break on the former it could break on the latter too, I'd think. |
|
Great work; This should be fixed in JitIL as well. |
|
JitIL fixed, I think. |
| @@ -38,7 +38,10 @@ void Jit64::psq_st(UGeckoInstruction inst) | |||
| ADD(32, R(ECX), Imm32((u32)offset)); | |||
| if (update && offset) | |||
| MOV(32, gpr.R(a), R(ECX)); | |||
| MOVZX(32, 16, EAX, M(&PowerPC::ppcState.spr[SPR_GQR0 + inst.I])); | |||
| // We need to mask out the unused bits; some games (e.g. Dirt 2) incorrectly | |||
| // set the unused bits which breaks the lookup table code. | |||
This comment was marked as off-topic.
This comment was marked as off-topic.
Sorry, something went wrong.
|
Just curious, have you looked at the offending Dirt2 code to see if it really intends to set those bits, or it could be a side effect of another bug? |
|
I don't know how I'd figure out whether or not Dirt2 was doing that; any suggestions? |
|
Okay, I wrote up some debug code and got this: 47:33:582 PowerPC\Interpreter\Interpreter_SystemRegisters.cpp:315 E[PowerPC]: GQR set to 80048004 Since the GQR isn't supposed to have bits other than 0x3F073F07 set, this looks like a mistake. |
While the unused bits in the GQR SPR are probably not supposed to be set, some games set them anyways (e.g. Dirt 2), which broke the JIT code.
|
Sure, but how are these bits being set? It might be a bug in our code somewhere. I still want to figure out if the hardware actually ignores these bits too. If so, it might make more sense to mask the value when the registers are actually set. |
|
Those log messages were directly from the interpreter, directly running game code. I don't see how it could be a bug in Dolphin; those are the values the game itself is trying to fill those registers with, unless I'm missing something about how SPRs work. |
|
Well, those values were presumably also calculated by the interpreter. There could be another bug in the interpreter causing it to emit those bogus values, which it's eventually trying to write to the GQRs. |
|
I guess the suggestion is obvious enough, but a hwtest would clear this up fairly easily, I guess :) |
|
I don't know how to do a hwtest (and I'm not back home with my Wii for another week, so I couldn't homebrew it even if I wanted to!) |
|
Look at the existing ones at https://github.com/dolphin-emu/hwtests/tree/master/cputest . If you can give me some PPC assembly code to perform such a test, I might be able to write one myself (chances are I fail too much for that, though :p) |
|
hwtest isn't the answer to my question (although it's a good idea for determining how writing those bits behaves [although it seems pretty clear they are just hardwired to 0]), it's figuring out how the values written to GQRs are calculated. Try finding the PC of the write in Dirt2 and tracing the taint back. |
|
I tried to track it down, but I was led only to a function that takes a memory buffer and fills the GQRs with it. I tried disabling most of the JIT and seeing if the problem remained (i.e. incorrect GQR value) and it did, so if it -is- a Dolphin bug generating those values, it's common to the interpreter too. |
|
Okay, the actual setting happens here: This looks really intentional; I have no idea what they're trying to do here. This is a GQR that was previously set to 0x00040004, and they're going to great effort to explicitly change it to 0x80048004. I can't imagine why... ... I really hope they're not using the upper bit as a flag or something for their own purposes |
|
Okay, I have a hwtest possibility, if anyone else can test it, based on this code: lis r3, 0x8005 My question is basically: is r0 (at the end) 0x80048004 or 0x00040004? |
|
Okay, flacs did a hwtest and it looks like the PPC does not internally zero the unused bits -- it just doesn't use them (storing and loading keeps the bits unchanged). |
|
https://gist.github.com/FioraAeterna/f142b01f4dc10cab687a Here's the hardware test, for reference. |
|
Since it looks like games actually intentionally set these bits (for some bizarre reason), and the CPU does not clear them, we should probably stick to the AND solution, instead of masking out the bits inside mtpsr, since the latter might be incorrect (if a game stores a flag in the unused part of the GQR, it'll want the flag to still be there when it reads it out later). |
JIT: Don't assume the reserved bits in GQRs are zero
While the unused bits in the GQR SPR are probably not supposed to be set, some
games set them anyways (e.g. Dirt 2), which broke the JIT code.