Refactor the fastmem/trampoline code #3496

mmastrac · 2016-01-11T00:44:54Z

Refactoring fastmem code to make it more maintainable. Note that this requires #3454 to land first (the two PRs are not reorderable).

Source/Core/Core/PowerPC/Jit64/Jit.cpp

+				 gqr_modified = true;
+				 break;
+			}
+		}


mmastrac · 2016-01-11T03:48:01Z

The Android build uses a struct with the same name -- I'll rename the x64 version to something else to fix that part of the build.

mmastrac · 2016-02-04T21:55:03Z

This code is no longer WIP and is ready for review/testing

delroth · 2016-02-04T21:59:14Z

FYI, I consider this a risky change and as such it won't be merged until 5.0 is out (sorry!).

mmastrac · 2016-02-04T22:01:21Z

No worries.. this is just queued up for post-5.0.

lioncash · 2016-02-04T22:15:37Z

Review status: 0 of 22 files reviewed at latest revision, 11 unresolved discussions.

Source/Core/Core/PowerPC/Jit64/Jit_LoadStorePaired.cpp, line 79 [r3] (raw file):
static_cast

Source/Core/Core/PowerPC/Jit64/Jit_LoadStorePaired.cpp, line 85 [r3] (raw file):
indentation looks a little off here.

Source/Core/Core/PowerPC/Jit64/Jit_LoadStorePaired.cpp, line 167 [r3] (raw file):
static_cast

Source/Core/Core/PowerPC/Jit64Common/Jit64AsmCommon.cpp, line 228 [r3] (raw file):
static_cast

Source/Core/Core/PowerPC/Jit64Common/Jit64AsmCommon.cpp, line 238 [r3] (raw file):
static_cast

Source/Core/Core/PowerPC/Jit64Common/Jit64AsmCommon.cpp, line 258 [r3] (raw file):
static_cast

Source/Core/Core/PowerPC/Jit64Common/Jit64AsmCommon.cpp, line 260 [r3] (raw file):
static_cast

Source/Core/Core/PowerPC/Jit64Common/Jit64AsmCommon.cpp, line 418 [r3] (raw file):
Is there a particular reason this cast was added? (M is a template function that takes any pointer type)

Source/Core/Core/PowerPC/JitCommon/Jit_Util.cpp, line 375 [r3] (raw file):
These should be reinterpret casts.

Source/Core/Core/PowerPC/JitCommon/JitBackpatch.cpp, line 121 [r3] (raw file):
static_cast

Source/Core/Core/PowerPC/JitCommon/JitBackpatch.cpp, line 124 [r3] (raw file):
static_cast

Comments from the review on Reviewable.io

BhaaLseN · 2016-02-05T15:11:35Z

Source looks good to me in general (apart from the few notes I left), but I cannot really comment much on the actual implementation since I'm not overly familiar with that part of the code base.

Reviewed 5 of 22 files at r1, 13 of 17 files at r3, 4 of 4 files at r4.
Review status: all files reviewed at latest revision, 18 unresolved discussions.

Source/Core/Core/PowerPC/Jit64/Jit.cpp, line 648 [r4] (raw file):
This single word on the next line looks odd, can you wrap the previous line differently to soothe my uncalmed mind? :)

Source/Core/Core/PowerPC/Jit64/Jit_LoadStorePaired.cpp, line 68 [r4] (raw file):
one what, pair what? As someone not overly familiar with this part of the code, I don't see the value or intent behind this comment. Either remove it (if its obvious when you know what you're doing) or be more verbose to get others on board.
But then again, I suppose you just moved the comment from the section down below that has been removed/rewritten. The previous comment made a little more sense being "Pair of values", but on its own (with w just around the corner) even that could be omitted I'd say.

Source/Core/Core/PowerPC/Jit64Common/Jit64AsmCommon.cpp, line 276 [r4] (raw file):
"a quantization factor in RSCRATCH2" feels like there are some words missing? Don't be afraid to write more verbose comments, it's bound to help someone one day (even yourself) when done properly, even when you might not see the immediate advantage of doing so.

Source/Core/Core/PowerPC/Jit64Common/Jit64AsmCommon.cpp, line 324 [r4] (raw file):
Is default an error or a no-op case? It should probably have an assert or a comment indicating this if you ask me.

Source/Core/Core/PowerPC/Jit64Common/Jit64AsmCommon.cpp, line 478 [r4] (raw file):
Same considerations for default as above.

Source/Core/Core/PowerPC/Jit64Common/Jit64AsmCommon.cpp, line 554 [r4] (raw file):
Here too.

Source/Core/Core/PowerPC/Jit64IL/JitIL.h, line 20 [r4] (raw file):
I don't see any changes to JitIL.cpp, has this include been missed before?

Comments from the review on Reviewable.io

Source/Core/Core/PowerPC/JitCommon/Jit_Util.cpp

-  // PC is used by memory watchpoints (if enabled) or to print accurate PC locations in debug logs
-  MOV(32, PPCSTATE(pc), Imm32(jit->js.compilerPC));
+    // PC is used by memory watchpoints (if enabled) or to print accurate PC locations in debug logs
+    MOV(32, PPCSTATE(pc), Imm32(jit->js.compilerPC));


magumagu · 2016-06-27T03:26:25Z

In general, it looks like you're making MemoryExceptionCheck itself check for DSI exceptions, rather than making the caller do it. I can't remember why I didn't do that myself the last time I was working with this code, but it should be fine.

Source/Core/Core/PowerPC/JitCommon/Jit_Util.cpp

@@ -18,6 +18,26 @@ using namespace Gen;

 void EmuCodeBlock::MemoryExceptionCheck()
 {
+  // TODO: We really should untangle the trampolines, exception handlers and
+  // memory checks.


mmastrac · 2016-06-27T03:38:17Z

Moving MemoryExceptionCheck was definitely another simplification. In the regcache patches I ended up removing all the callers outside of Jit_Util.cpp.

magumagu · 2016-06-27T03:41:27Z

LGTM.

degasus · 2016-06-27T17:20:02Z

Reviewed 2 of 22 files at r1, 16 of 20 files at r5, 1 of 1 files at r6.
Review status: 19 of 20 files reviewed at latest revision, 14 unresolved discussions.

Source/Core/Core/PowerPC/JitCommon/Jit_Util.h, line 94 [r7] (raw file):

  // Set to true if we added the offset to the address and need to undo it
  bool offsetAddedToAddress;

please move this bool to the other bools

Source/Core/Core/PowerPC/JitCommon/Jit_Util.h, line 207 [r7] (raw file):

protected:
  std::unordered_map<u8*, TrampolineInfo> backPatchInfo;

This map sounds quite big. Do you think it's worth to keep a set of all trampoline infos? Maybe also with a map to not generate them redundantly. Also the members of the struct are in a bad order with a lot of padding.
To be honest, no idea if this is worth.....

Nevermind, I doubt this is possible because of the DSI exception handler.

Another idea: The TrampolineInfo seems to be quite big (50+ bytes), do you think the actual loading code is smaller? If so, we should always generate the slow path and avoid storing the info here.

Source/Core/Core/PowerPC/JitCommon/JitBackpatch.cpp, line 73 [r7] (raw file):

  // Generate the trampoline.
  const u8* trampoline;
  if (info.read)

Please move this if/else into trampolines.GenerateTrampoline(info);

Source/Core/Core/PowerPC/JitCommon/JitBase.h, line 15 [r7] (raw file):

#include "Common/CommonTypes.h"
#include "Common/x64ABI.h"

Why is this header required?

Comments from Reviewable

mmastrac · 2016-06-27T18:09:44Z

Review status: 13 of 20 files reviewed at latest revision, 9 unresolved discussions.

Source/Core/Core/PowerPC/Jit64Common/Jit64AsmCommon.cpp, line 554 [r4] (raw file):

Previously, BhaaLseN (BhaaL) wrote…

Here too.

(resolved a bunch of these comments since they technically belonged to code that was moved to a diff PR)

Source/Core/Core/PowerPC/JitCommon/Jit_Util.h, line 94 [r7] (raw file):

Previously, degasus (Markus Wick) wrote…

please move this bool to the other bools

Done.

Source/Core/Core/PowerPC/JitCommon/Jit_Util.h, line 207 [r7] (raw file):

Previously, degasus (Markus Wick) wrote…

This map sounds quite big. Do you think it's worth to keep a set of all trampoline infos? Maybe also with a map to not generate them redundantly. Also the members of the struct are in a bad order with a lot of padding.
To be honest, no idea if this is worth.....

Nevermind, I doubt this is possible because of the DSI exception handler.

Another idea: The TrampolineInfo seems to be quite big (50+ bytes), do you think the actual loading code is smaller? If so, we should always generate the slow path and avoid storing the info here.

I've packed the struct and removed any redundant info. The struct is now 64 bytes.

Source/Core/Core/PowerPC/JitCommon/JitBackpatch.cpp, line 121 [r3] (raw file):

Previously, lioncash (Mat M.) wrote…

static_cast

Done.

Source/Core/Core/PowerPC/JitCommon/JitBackpatch.cpp, line 124 [r3] (raw file):

Previously, lioncash (Mat M.) wrote…

static_cast

Done.

Source/Core/Core/PowerPC/JitCommon/JitBackpatch.cpp, line 73 [r7] (raw file):

Previously, degasus (Markus Wick) wrote…

Please move this if/else into trampolines.GenerateTrampoline(info);

Done.

Source/Core/Core/PowerPC/JitCommon/JitBase.h, line 15 [r7] (raw file):

Previously, degasus (Markus Wick) wrote…

Why is this header required?

Not sure. I'll see if it is required to build.

Comments from Reviewable

degasus · 2016-06-27T20:10:27Z

Reviewed 1 of 20 files at r5, 6 of 6 files at r8.
Review status: all files reviewed at latest revision, 4 unresolved discussions.

Comments from Reviewable

Simplication to avoid reading back the generated instructions, allowing us to handle all possible cases.

Refactor the fastmem/trampoline code # Conflicts: # Source/Core/Core/PowerPC/JitCommon/JitBackpatch.cpp

lioncash reviewed Jan 11, 2016
View reviewed changes

Source/Core/Core/PowerPC/Jit64/Jit.cpp

gqr_modified = true;

break;

}

}

This comment was marked as off-topic.

Sign in to view

This comment was marked as off-topic.

Sign in to view

This comment was marked as off-topic.

Sign in to view

mmastrac force-pushed the fastmem_refactor branch from 40be166 to 7905e5e Compare January 11, 2016 03:45

CrossVR added the WIP / do not merge Work in progress (do not merge) label Jan 12, 2016

mmastrac force-pushed the fastmem_refactor branch 3 times, most recently from f3e91ba to 3dd3018 Compare February 4, 2016 21:50

mmastrac changed the title ~~Fastmem refactoring (WIP DO NOT MERGE)~~ Refactor the fastmem/trampoline code Feb 4, 2016

mmastrac force-pushed the fastmem_refactor branch from 3dd3018 to 81ee61d Compare February 4, 2016 22:05

mmastrac force-pushed the fastmem_refactor branch from 81ee61d to e786cd9 Compare February 4, 2016 23:13

mmastrac force-pushed the fastmem_refactor branch 3 times, most recently from c0254f9 to fcef1a7 Compare June 27, 2016 02:20

mmastrac force-pushed the fastmem_refactor branch from fcef1a7 to 419db71 Compare June 27, 2016 02:38

magumagu reviewed Jun 27, 2016
View reviewed changes

mmastrac force-pushed the fastmem_refactor branch from 419db71 to 9e42b24 Compare June 27, 2016 03:29

magumagu reviewed Jun 27, 2016
View reviewed changes

mmastrac mentioned this pull request Jun 27, 2016

[WIP/RFC] Refactor the register cache to use C++ idioms for locking/binding/unlocking #3565

Closed

mmastrac force-pushed the fastmem_refactor branch from 9e42b24 to a359a13 Compare June 27, 2016 16:38

degasus removed the WIP / do not merge Work in progress (do not merge) label Jun 27, 2016

mmastrac force-pushed the fastmem_refactor branch from f555f4c to ae999c1 Compare June 27, 2016 20:26

Refactor fastmem/trampoline code.

b1296a7

Simplication to avoid reading back the generated instructions, allowing us to handle all possible cases.

mmastrac force-pushed the fastmem_refactor branch from ae999c1 to b1296a7 Compare June 27, 2016 20:58

degasus merged commit adcef04 into dolphin-emu:master Jun 27, 2016

CarlKenner pushed a commit to CarlKenner/dolphin that referenced this pull request Jun 28, 2016

Merge pull request dolphin-emu#3496 from mmastrac/fastmem_refactor

e1e47c2

Refactor the fastmem/trampoline code # Conflicts: # Source/Core/Core/PowerPC/JitCommon/JitBackpatch.cpp

phire mentioned this pull request Jun 30, 2016

Fix broken debug builds in visual studio #3966

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor the fastmem/trampoline code #3496

Refactor the fastmem/trampoline code #3496

mmastrac commented Jan 11, 2016 •

edited

This comment was marked as off-topic.

This comment was marked as off-topic.

This comment was marked as off-topic.

mmastrac commented Jan 11, 2016

mmastrac commented Feb 4, 2016

delroth commented Feb 4, 2016

mmastrac commented Feb 4, 2016

lioncash commented Feb 4, 2016

BhaaLseN commented Feb 5, 2016

This comment was marked as off-topic.

This comment was marked as off-topic.

magumagu commented Jun 27, 2016

This comment was marked as off-topic.

This comment was marked as off-topic.

mmastrac commented Jun 27, 2016

magumagu commented Jun 27, 2016

degasus commented Jun 27, 2016

mmastrac commented Jun 27, 2016

degasus commented Jun 27, 2016

Refactor the fastmem/trampoline code #3496

Refactor the fastmem/trampoline code #3496

Conversation

mmastrac commented Jan 11, 2016 • edited

This comment was marked as off-topic.

This comment was marked as off-topic.

This comment was marked as off-topic.

mmastrac commented Jan 11, 2016

mmastrac commented Feb 4, 2016

delroth commented Feb 4, 2016

mmastrac commented Feb 4, 2016

lioncash commented Feb 4, 2016

BhaaLseN commented Feb 5, 2016

This comment was marked as off-topic.

This comment was marked as off-topic.

magumagu commented Jun 27, 2016

This comment was marked as off-topic.

This comment was marked as off-topic.

mmastrac commented Jun 27, 2016

magumagu commented Jun 27, 2016

degasus commented Jun 27, 2016

mmastrac commented Jun 27, 2016

degasus commented Jun 27, 2016

mmastrac commented Jan 11, 2016 •

edited