Refactor opcode decoding a bit to kill FifoCommandRunnable. #924
Conversation
| { | ||
| Decode(); | ||
| } | ||
| OpcodeDecoder_Run(false, end); |
This comment was marked as off-topic.
This comment was marked as off-topic.
Sorry, something went wrong.
This comment was marked as off-topic.
This comment was marked as off-topic.
Sorry, something went wrong.
Separated out from my gpu-determinism branch by request. It's not a big commit; I just like to write long commit messages. The main reason to kill it is hopefully a slight performance improvement from avoiding the double switch (especially in single core mode); however, this also improves cycle calculation, as described below. - FifoCommandRunnable is removed; in its stead, Decode returns the number of cycles (which only matters for "sync" GPU mode), or 0 if there was not enough data, and is also responsible for unknown opcode alerts. Decode and DecodeSemiNop are almost identical, so the latter is replaced with a skipped_frame parameter to Decode. Doesn't mean we can't improve skipped_frame mode to do less work; if, at such a point, branching on it has too much overhead (it certainly won't now), it can always be changed to a template parameter. - FifoCommandRunnable used a fixed, large cycle count for display lists, regardless of the contents. Presumably the actual hardware's processing time is mostly the processing time of whatever commands are in the list, and with this change InterpretDisplayList can just return the list's cycle count to be added to the total. (Since the calculation for this is part of Decode, it didn't seem easy to split this change up.) To facilitate this, Decode also gains an explicit 'end' parameter in lieu of FifoCommandRunnable's call to GetVideoBufferEndPtr, which can point to there or to the end of a display list (or elsewhere in gpu-determinism, but that's another story). Also, as a small optimization, InterpretDisplayList now calls OpcodeDecoder_Run rather than having its own Decode loop, to allow Decode to be inlined (haven't checked whether this actually happens though). skipped_frame mode still does not traverse display lists and uses the old fake value of 45 cycles. degasus has suggested that this hack is not essential for performance and can be removed, but I want to separate any potential performance impact of that from this commit.
comex
force-pushed
the
fifo-command-runnable
branch
from
21c143f
to
608f9bc
Compare
|
I didn't realize how tiny the difference between Decode and DecodeSemiNop actually was... so as suggested by degasus, I updated the patch to just use one function, and removed the enum. (Because the decoding logic is all the same, I think I should just update gpu-determinism's DecodePreprocess to be a template specialization of Decode...) As mentioned in the commit message, even though it probably shouldn't skip display lists anyway, this patch is conservative and doesn't change behavior, except for the display list cycle counting which seems obviously better. Also fixed style and the missing |
| @@ -173,7 +173,7 @@ void RunGpuLoop() | |||
|
|
|||
| ReadDataFromFifo(uData, 32); | |||
|
|
|||
| cyclesExecuted = OpcodeDecoder_Run(g_bSkipCurrentFrame); | |||
| cyclesExecuted = OpcodeDecoder_Run(g_bSkipCurrentFrame, GetVideoBufferEndPtr()); | |||
This comment was marked as off-topic.
This comment was marked as off-topic.
Sorry, something went wrong.
This comment was marked as off-topic.
This comment was marked as off-topic.
Sorry, something went wrong.
|
LGTM @JMC47 Do you want to do some testing on this PR? flacs has told me his last version still has some issues, are this issue also valid here? |
|
I did test it a bit, but I don't know what issues I'm looking for; it seemed to run games/benchmarks just fine. |
|
Here goes nothing. |
Refactor opcode decoding a bit to kill FifoCommandRunnable.
|
This pr "breaks" the sync gpu option, was that intended? When playing Fzero gx with sync gpu and start a campaign race it runs in slow motion since it gives 20fps as 100% speed for a 60fps game. |
|
Well... breaking it is obviously not intended, but the new calculation is theoretically more accurate than the old calculation. File an issue please... |
|
the commit 608f9bc caused this issue: https://code.google.com/p/dolphin-emu/issues/detail?id=8104 |
Separated out from my gpu-determinism branch by request. It's not a big
commit; I just like to write long commit messages.
The main reason to kill it is hopefully a slight performance improvement
from avoiding the double switch (especially in single core mode);
however, this also improves cycle calculation, as described below.
return the number of cycles (which only matters for "sync" GPU mode), or
0 if there was not enough data, and are also responsible for unknown
opcode alerts.
To minimize duplication, the actual cycle counts are stored in an enum,
but some still exists. In the future it would probably be a good idea
to merge Decode and DecodeSemiNop into one function, because they're
very similar.
regardless of the contents. Presumably the actual hardware's processing
time is mostly the processing time of whatever commands are in the list,
and with this change InterpretDisplayList can just return the list's
cycle count to be added to the total. (Since the calculation for this
is part of Decode, it didn't seem easy to split this change up.)
To facilitate this, Decode and DecodeSemiNop also gain an explicit 'end'
parameter in lieu of FifoCommandRunnable's call to GetVideoBufferEndPtr,
which can point to there or to the end of a display list (or elsewhere
in gpu-determinism, but that's another story). Also, as a small
optimization, InterpretDisplayList now calls OpcodeDecoder_Run rather
than having its own Decode loop, to allow Decode to be inlined (haven't
checked whether this actually happens though).
DecodeSemiNop, which doesn't traverse display lists at all, still uses
the old fake value of 45 cycles. There's probably some way to improve
this.