Simplifications to the state access mechanism #303
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
edubart
reviewed
Jan 7, 2025
mpernambuco
previously approved these changes
Jan 7, 2025
diegonehab
force-pushed
the
refactor/more-simplifications
branch
2 times, most recently
from
cda4fbd to
8bd1b59
Compare
The shadow PMAs never change once the machine is running. No need to use the more complicated device mechanism, a read-only memory range is fine. This allows us to use the standard "peek" for memory ranges with the Mekrle tree. The old peek needed to use the device-state-access to build the shadow from scratch every time. It used i-device-state-access, which needed to have read_pma_istart/read_pma_ilength. This in turn required us to add read_pma_istart/read_pma_ilength to i-state-access. A huge waste of effort for something that can simply be done in machine.cpp once and for all.
diegonehab
force-pushed
the
refactor/more-simplifications
branch
3 times, most recently
from
d5cb327 to
dff530d
Compare
edubart
previously approved these changes
Jan 8, 2025
diegonehab
force-pushed
the
refactor/more-simplifications
branch
from
dff530d to
113ea9e
Compare
A single unified implementation is enough.
Remove context from HTIF device. This is important because it was the only device used in reproducible code that had a context. This meant there was some gambiarra in the driver code to check if the context was nullptr. Remove read_device and write_device from i_state_access. The code is now in interpret.cpp. This reduces the amount of code we need when implementing a new state access class. Simplify the mock pma implementations used by different state access classes. I think we should merge them into a single one.
diegonehab
force-pushed
the
refactor/more-simplifications
branch
from
113ea9e to
bec0525
Compare
edubart
approved these changes
Jan 9, 2025
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
The shadow PMAs never change once the machine is running.
No need to use the more complicated device mechanism, a read-only memory range is fine.
This allows us to use the standard "peek" for memory ranges with the Mekrle tree.
The old peek needed to use the device-state-access to build the shadow from scratch every time.
It used i-device-state-access, which needed to have read_pma_istart/read_pma_ilength.
This in turn required us to add read_pma_istart/read_pma_ilength to i-state-access.
A waste of effort for something that can simply be done in machine.cpp once and for all.
We also remove find_pma_entry from the state access.
A single implementation in interpret.cpp is enough.
Remove context from HTIF device.
This is important because it was the only device used in reproducible code that had a context.
This meant there was some gambiarra in the driver code to check if the context was nullptr.
Remove read_device and write_device from i_state_access.
The code is now in interpret.cpp.
This reduces even more the amount of code we need when implementing a new state access class.
Simplify the mock pma implementations used by different state access classes.
I think we should merge them into a single one.