-
Notifications
You must be signed in to change notification settings - Fork 34
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
MmioPatterns optimisation #1607
base: arith-dev
Are you sure you want to change the base?
Changes from 8 commits
d06727b
521fc7b
596d750
7beaec4
905f5b3
8292ad6
9dc21be
e2eae6e
ee0fa20
59734b9
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -182,17 +182,18 @@ public static Bytes16 excision( | |
|
||
public static void updateTemporaryTargetRam( | ||
MmuData mmuData, final long targetLimbOffsetToUpdate, final Bytes16 newLimb) { | ||
final Bytes bytesPreLimb = | ||
Bytes.repeat( | ||
(byte) 0, | ||
(int) | ||
(LLARGE | ||
* targetLimbOffsetToUpdate)); // We won't access the preLimb again, so we don't | ||
// care | ||
// of its value | ||
final Bytes bytesPostLimb = | ||
mmuData.targetRamBytes().slice((int) ((targetLimbOffsetToUpdate + 1) * LLARGE)); | ||
|
||
mmuData.targetRamBytes(Bytes.concatenate(bytesPreLimb, newLimb, bytesPostLimb)); | ||
|
||
int limbStart = (int) (LLARGE * targetLimbOffsetToUpdate); | ||
int limbEnd = limbStart + LLARGE; | ||
byte[] originalRam = mmuData.targetRamBytes().toArray(); | ||
int sliceSize = Math.max(0, originalRam.length - limbEnd); | ||
|
||
int size = limbStart + newLimb.size() + sliceSize; | ||
byte[] updatedRam = new byte[size]; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Here the updated ram size should be the same as the original one ram There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What's this method you're modifying is doing : you just want to modify a 16 bytes limb the midldle of your memory so :
Not sure of what you're doing, I'm on mobile with low network There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
In my current implementation, it is the case, but any way, we need to follow this logic. The total size is the sum of the parts' sizes that we're putting together. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
I'm doing the same work, but this implementation is faster as you can see in the flamegraphs |
||
System.arraycopy(newLimb.toArray(), 0, updatedRam, limbStart, newLimb.size()); | ||
if (sliceSize > 0) | ||
System.arraycopy(originalRam, limbEnd, updatedRam, limbStart + newLimb.size(), sliceSize); | ||
|
||
mmuData.targetRamBytes(Bytes.wrap(updatedRam)); | ||
} | ||
} |
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. this should not be updated. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Not sure why it was updated, it changed when I checkout the project. I thought it was expected. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Just point to latest master of constraint and it'll be ok. The tracer is currently pointing to a commit that disables the MMIO (needed for the release) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe we could add a simple unit test to validate the method?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed, @letypequividelespoubelles, I will need some help on creating a unit test as we don't have one before. I don't visualize yet the real use case.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Or maybe @OlivierBBB, as Francois is OOO currently.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is needed when a (target) limb is updated in two times by a MMU instructions. It's the case for example for MMU instructions generating *TwoTarget MMIO instructions.
We already have some memory tests that triggers those MMIO inst.
Maybe this is the failing unit tests (not much network at the hospital, can't open the test output), but I guess it's MMIO.ram_excision or MMIO.*_to_ram_two_targets constraints that are failing.