Added simple sparese memory implementation.
Review Request #473 - Created Feb. 11, 2011 and discarded
| Information | |
|---|---|
| Ali Saidi | |
| gem5 | |
| Reviewers | |
| Default | |
| ali, gblack, nate, stever | |
Added simple sparese memory implementation. For general modeling purposes, it is useful to have a simple memory container which allocates storage on demand, allowing for scalable host memory usage. This patch adds sparse_mem.hh/cc to src/base to implement such a general purpose container object. The ELF loader and ObjectFile classes were also extended to support loading SparseMem with ELF images.
Posted (Feb. 11, 2011, 11:37 a.m.)
While I like the idea (we had something like this long ago as an alternative to PhysicalMemory and I think it would be really nice to support that again), it's not clear at all how you intend to use it. The duplication of the load functions seems odd. Why would SparseMem not be behind a port interface? Also, This just doesn't look like M5 code. Can you do a style pass to clean it up and make it look more like what we have in the tree? There's lots of stuff that is inconsistent even though we don't have that stuff in the style guide.
-
src/base/sparse_mem.hh (Diff revision 1) -
should be __BASE_SPARSE_MEM_HH__
-
src/base/sparse_mem.hh (Diff revision 1) -
Probably should include "base/types.hh" instead
-
src/base/sparse_mem.hh (Diff revision 1) -
Brace on new line.
-
src/base/sparse_mem.hh (Diff revision 1) -
Non-standard use of spaces
-
src/base/sparse_mem.hh (Diff revision 1) -
why is this virtual? Are you expecting to use polymorphism?
-
src/base/sparse_mem.hh (Diff revision 1) -
excess whitespace.
-
src/base/sparse_mem.cc (Diff revision 1) -
should be "base/sparse_mem.hh" also this should go below system includes.
-
src/base/sparse_mem.cc (Diff revision 1) -
why not just return early here so you don't have to have such a huge else block?
Posted (Feb. 11, 2011, 2:51 p.m.)
I agree with Nate that this should be behind a port interface. It should be a MemObject like phys mem, configured as a simobject, etc. It's not clear how it's actually used as is, other than being loaded from an object file. The code duplication is a serious problem, and I strongly object to it being committed while that's in there. The read and write functions can be simplified (see below), and their interface can be improved by, for instance, not allocating memory for the return value for read. Using ports would clean the interface up automatically, I think. I also agree this would be nice to have once it gets fixed up. Some ideas for improvement that wouldn't necessarily need to go into this change: 1. Reads always return 0 if the block is unallocated, only allocate on writes. 2. Have a maximum size in memory, and if too many blocks get accessed swap some out to files.
-
src/base/sparse_mem.hh (Diff revision 1) -
These should probably either be protected instead of private or have accessors. Subclasses would probably need to know these things. It's not highly likely there would -be- subclasses, but making it easier to add them is almost free.
-
src/base/sparse_mem.hh (Diff revision 1) -
I'm being a little anal, but blockSize isn't a great name for this value. It's not the block size, it's really the log of the block size. That might confuse people. It would be better, I think, to take this as an expanded value and then verify that it's a power of two and complain if it isn't. That would make it more natural to use, make the name fit better, and avoid having to compute 1 << blockSize in a place or two.
-
src/base/sparse_mem.cc (Diff revision 1) -
Why are you using dynamic memory here? Why not pass in a pointer to copy into?
-
src/base/sparse_mem.cc (Diff revision 1) -
This whole thing could be restructured as a while loop. while (bytes left) { bytes = min(bytes left, bytes left in block) there = block to write to memcpy(there, here, bytes) here += bytes bytes left -= bytes }
