-
Notifications
You must be signed in to change notification settings - Fork 906
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
feat: add a dense memory backend implementation #1342
feat: add a dense memory backend implementation #1342
Conversation
5ed5f1b
to
32acdef
Compare
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 does far too much in one commit:
- Adding command-line option
:d
- Creating class hierarchy in
mem_t
- Adding
dense
tomem_cfg_t
- Implementing
dense_mem_t
That's at least 4 separate commits.
The bugfix just pushed should be squashed back in so there is no bug in any commit.
But more importantly, I think this needs more justification. What is the benefit here? Does this improve performance when simulating programs that actually use a large block of memory? By how much?
riscv/devices.h
Outdated
sparse_mem_t(reg_t size); | ||
~sparse_mem_t(); | ||
|
||
char* contents(reg_t addr); |
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.
Use virtual
and override
keywords on overridden virtual methods
Hi Scott, Thanks for reviewing my PR. I'll address your comments in subsequent commits. I'll split this single commit as you suggested. The benefit is that the current lazy allocation scheme is inefficient compared to allocating the entire backing memory at once. For example, in ucb-bar/chipyard#1438, we unify the backing memory in both spike cosim and the actual RTL simulation to detect divergence. At the start of the cosimulation, the entire RTL memory is copied into spike, which in turn calls calloc many times. The other reason is so that this could serve as the backing memory for a block device for using with drivers like https://github.com/u-boot/u-boot/blob/master/drivers/mmc/piton_mmc.c. If you'd like I could run some benchmark comparison on memcpy benchmarks. Thanks, |
32acdef
to
ca08a1f
Compare
I suspect we can improve the current scheme to be more efficient, e.g. by using memalign and allocating larger chunks (say, 2 MiB to match the superpage size). This would require some experimentation, but it seems preferable to having two different schemes. I suspect the block device will end up being a separate |
e2300a3
to
286f8b5
Compare
I would argue that multiple backends are beneficial, and is also practical in other projects as qemu: https://github.com/qemu/qemu/blob/master/backends/hostmem.c. For one, the dense allocation will crash if allocated physical memory is larger than host memory, but the sparse backend could effectively test memory address > 39 bits for example. The dense allocation will also make sharing memory easier and more flexible instead of performing a lookup. |
@tianrui-wei for our use case, we can just provide our own implementation of mem_t that uses a dense memory in our code that links with spike, and pass it to the sim_t constructor. |
Could we perhaps only cherry pick e71df64 that implements an abstract class for memory interface, and expose a flexible interface in libriscv? |
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.
Commit e71df64 fails to compile:
/local_home/sjohnson/spike-regress/riscv-isa-sim/spike_main/spike.cc: In function ‘std::vector<std::pair<long unsigned int, mem_t*> > make_mems(const std::vector<mem_cfg_t>&)’:
/local_home/sjohnson/spike-regress/riscv-isa-sim/spike_main/spike.cc:273:75: error: invalid new-expression of abstract class type ‘mem_t’
mems.push_back(std::make_pair(cfg.get_base(), new mem_t(cfg.get_size())));
^
3d6c41b
to
69e70ad
Compare
Hi Scott, Thank you for reviewing and shepherding the PR. I've updated the commit to address your reviews and only cherry-picked the first commit. Thanks, |
69e70ad
to
30a0169
Compare
Signed-off-by: Tianrui Wei <tianrui@tianruiwei.com>
30a0169
to
b72cf05
Compare
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.
I'll leave it up to @aswaterman but this seems like a pointless change now.
virtual void dump(std::ostream& o) override; | ||
|
||
private: | ||
bool load_store(reg_t addr, size_t len, uint8_t* bytes, bool store) override; |
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.
Needs virtual
Resolved by #1408 |
Currently, the mem_t only supports a sparse memory implementation. This
commit adds in dense memory support. It behaves as follows:
in modern Linux kernels
on demand
Signed-off-by: Tianrui Wei tianrui@tianruiwei.com