Oh, also, you have a type in your summary line (sparese). On 02/11/11 22:51, Gabe Black wrote: > This is an automatically generated e-mail. To reply, visit: > http://reviews.m5sim.org/r/473/ > > > 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/diff/1/?file=10284#file10284line61> > (Diff revision 1) > > > class SparseMem { > > > 61 > uint8_t blockSize; > > 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/diff/1/?file=10284#file10284line72> > (Diff revision 1) > > > class SparseMem { > > > 72 > * @param _blockSize Memory is allocated in 2^_blockSize byte chunks. > > 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/diff/1/?file=10285#file10285line87> > (Diff revision 1) > > > SparseMem::read(uint64_t address, uint64_t numBytes) > > > 87 > uint8_t *data = new uint8_t[numBytes]; // allocate storage > > 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/diff/1/?file=10285#file10285line140> > (Diff revision 1) > > > SparseMem::read(uint64_t address, uint64_t numBytes) > > > 140 > if (topAddress <= blockEnd) { > > 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 February 11th, 2011, 4:43 p.m., Ali Saidi wrote: > > Review request for Default, Ali Saidi, Gabe Black, Steve Reinhardt, > and Nathan Binkert. > By Ali Saidi. > > /Updated 2011-02-11 16:43:37/ > > > Description > > 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) > > View Diff <http://reviews.m5sim.org/r/473/diff/> > > > _______________________________________________ > m5-dev mailing list > m5-dev@m5sim.org > http://m5sim.org/mailman/listinfo/m5-dev
_______________________________________________ m5-dev mailing list m5-dev@m5sim.org http://m5sim.org/mailman/listinfo/m5-dev
