----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: http://reviews.m5sim.org/r/473/#review845 -----------------------------------------------------------
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 <http://reviews.m5sim.org/r/473/#comment1215> 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 <http://reviews.m5sim.org/r/473/#comment1216> 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 <http://reviews.m5sim.org/r/473/#comment1213> Why are you using dynamic memory here? Why not pass in a pointer to copy into? src/base/sparse_mem.cc <http://reviews.m5sim.org/r/473/#comment1214> 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 } - Gabe On 2011-02-11 16:43:37, Ali Saidi wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > http://reviews.m5sim.org/r/473/ > ----------------------------------------------------------- > > (Updated 2011-02-11 16:43:37) > > > Review request for Default, Ali Saidi, Gabe Black, Steve Reinhardt, and > Nathan Binkert. > > > Summary > ------- > > 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. > > > Diffs > ----- > > src/base/SConscript 6548721032fa > src/base/loader/elf_object.hh 6548721032fa > src/base/loader/elf_object.cc 6548721032fa > src/base/loader/object_file.hh 6548721032fa > src/base/loader/object_file.cc 6548721032fa > src/base/sparse_mem.hh PRE-CREATION > src/base/sparse_mem.cc PRE-CREATION > > Diff: http://reviews.m5sim.org/r/473/diff > > > Testing > ------- > > > Thanks, > > Ali > >
_______________________________________________ m5-dev mailing list m5-dev@m5sim.org http://m5sim.org/mailman/listinfo/m5-dev
