Use C++20 features to create opcode tables at compile time #11400
Conversation
Pokechu22
force-pushed
the
better-ppc-tables
branch
4 times, most recently
from
8f14b94
to
fe24183
Compare
|
M1, Crashes when starting a game. Error & backtrace: |
|
Also affects x86-64 windows. I guess I didn't test enough after one of my rebases an broke something. |
| void Interpreter::RunTable4(UGeckoInstruction inst) | ||
| { | ||
| s_interpreter_op_table4[inst.SUBOP10](inst); | ||
| } | ||
| void Interpreter::RunTable19(UGeckoInstruction inst) | ||
| { | ||
| s_interpreter_op_table4[inst.SUBOP10](inst); | ||
| } | ||
| void Interpreter::RunTable31(UGeckoInstruction inst) | ||
| { | ||
| s_interpreter_op_table4[inst.SUBOP10](inst); | ||
| } | ||
| void Interpreter::RunTable59(UGeckoInstruction inst) | ||
| { | ||
| s_interpreter_op_table4[inst.SUBOP5](inst); | ||
| } | ||
| void Interpreter::RunTable63(UGeckoInstruction inst) | ||
| { | ||
| s_interpreter_op_table4[inst.SUBOP10](inst); | ||
| } |
There was a problem hiding this comment.
Here's the source of the problem; I use table4 for all of these by accident. The JITs are fine it seems. Although I'm not sure why this would be getting ran on the JITs; the whole splitting should mean that the JIT doesn't need to look at the interpreter at all now unless it's actually falling back.
There was a problem hiding this comment.
While the JIT doesn't run instructions that require a fallback all that often, there is a non-trivial number of instructions that require a fallback, so it sounds plausible to me that you would run into some of them at least once when playing a game.
Also, dvessel's stack trace was from while the apploader was running. We always use the interpreter for that.
| Interpreter::Instruction Interpreter::GetInterpreterOp(UGeckoInstruction inst) | ||
| { | ||
| // Check for the appropriate subtable ahead of time. | ||
| // (This is used by the cached interpreter and JIT, and called once per instruction, so spending a | ||
| // bit of extra time to optimise is worthwhile) | ||
| Interpreter::Instruction result = s_interpreter_op_table[inst.OPCD]; | ||
| if (result == Interpreter::RunTable4) | ||
| return s_interpreter_op_table4[inst.SUBOP10]; | ||
| else if (result == Interpreter::RunTable19) | ||
| return s_interpreter_op_table19[inst.SUBOP10]; | ||
| else if (result == Interpreter::RunTable31) | ||
| return s_interpreter_op_table31[inst.SUBOP10]; | ||
| else if (result == Interpreter::RunTable59) | ||
| return s_interpreter_op_table59[inst.SUBOP5]; | ||
| else if (result == Interpreter::RunTable63) | ||
| return s_interpreter_op_table63[inst.SUBOP10]; | ||
| else | ||
| return result; | ||
| } |
There was a problem hiding this comment.
... and a second, much more insidious thing also happens: because RunTable4, RunTable19, RunTable31, and RunTable63 all had the same body (RunTable59 used SUBOP5 so it was still different), the compiler is allowed to treat them as the same function, and then they end up having the same address, so this also ended up doing the wrong thing. However, the compiler is not required to treat them as the same function, so this only happened on release builds, not debug builds.
On x86-64, GetInterpreterOp was getting called from a 7c1083a6 mtspr IBAT0U, r0 instruction (and the JIT falls back to the interpreter for all but a few common mtspr instructions). So that's how this was getting called.
The instruction you had is a blr, which doesn't have the same fallback behavior, but because we don't use the JIT while running the apploader (compare #10923) the same previously mentioned issue with the RunTable functions applied (RunInterpreterOp would be used in that path, not GetInterpreterOp).
Pokechu22
force-pushed
the
better-ppc-tables
branch
2 times, most recently
from
efc327d
to
2ebf80c
Compare
|
No longer crashing. Looks good~! |
|
just curious: for |
|
I assume it's a limitation of their tools, as other C++20 features do work properly for the most part ( |
|
It seems It looks like they just picked a random pre-release commit from before 14.0 was actually released :( https://github.com/android/ndk/wiki#release-schedule and the next version of ndk is not expected to be released until end of 2023....really, not great. (however, they say non-LTS builds are released quarterly, so maybe there is hope for updating to one of those beforehand?) |
Pokechu22
force-pushed
the
better-ppc-tables
branch
from
2ebf80c
to
3594899
Compare
Pokechu22
force-pushed
the
better-ppc-tables
branch
from
3594899
to
68c3eeb
Compare
Pokechu22
force-pushed
the
better-ppc-tables
branch
from
68c3eeb
to
6911b6d
Compare
|
|
||
| for (auto& tpl : s_primary_table) | ||
| { | ||
| ASSERT(table[tpl.opcode] == &Jit64::FallBackToInterpreter); |
There was a problem hiding this comment.
I'm kinda surprised our ASSERT macro works in a consteval context, to be honest. But I guess it's fine if it works?
There was a problem hiding this comment.
If the ASSERT fails, then compilation fails, which is the behavior we want. The message in that case isn't that great, so it might make sense to add some kind of CONSTEVAL_ASSERT macro instead, but I don't think it's that important. (static_assert didn't seem to behave in a useful manner when I tested it, though I don't remember exactly why.)
|
This seems to break booting Kirby Air Ride (E) in Interpreter, but not Cached Interpreter weirdly enough. Lemme see if I can figure out why... |
|
It's 6911b6d, you changed behavior here. |
| const GekkoOPInfo* opinfo = PPCTables::GetOpInfo(m_prev_inst); | ||
|
|
||
| if (HandleFunctionHooking(PowerPC::ppcState.pc)) | ||
| { | ||
| UpdatePC(); | ||
| return PPCTables::GetOpInfo(m_prev_inst)->numCycles; | ||
| return opinfo->num_cycles; | ||
| } |
There was a problem hiding this comment.
Hmm, does that mean that this code is incorrect, as it meant that a hooked function uses the cycle count for the previous instruction instead of the current instruction? Not that exact cycle counts mean anything if we're hooking a function.
There was a problem hiding this comment.
I think you're right, yeah, at least I don't see any place m_prev_inst would be updated in this case to point at the current instruction. Probably better fixed in another PR though.
There was a problem hiding this comment.
Regression fixed. I've also added a TODO for this code.
Since OPLOG is defined in PPCTables.cpp only, it isn't visible elsewhere. This broke in 3ede866.
This also allows use of constexpr in both places. Some additional work was needed in PPCTables due to mutable data associated with each opcode.
We don't have getters for other flags, so it's not useful to have that.
Pokechu22
force-pushed
the
better-ppc-tables
branch
from
6911b6d
to
4dd658f
Compare
Previously, Interpreter_Tables.cpp contained both the information for running an instruction in the interpreter AND the flags used for that instruction everywhere (including the JITs). Furthermore, this couldn't be
const, as the last 3 members ofGekkoOPInfowere mutable (for statistics purposes) (and also, those 3 members were explicitly set to 0 on all instructions, even though that wasn't useful information)).Using C++20 features (
constevalas well as(Android still doesn't support this)) these are now created at compile time, and also verify that there are no duplicate entries. The stats portion ofconstexprstd::array::fillGekkoOPInfois now handled by a pointer to a separate array which allows the rest of it to beconst.