----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: http://reviews.m5sim.org/r/473/#review842 -----------------------------------------------------------
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 <http://reviews.m5sim.org/r/473/#comment1204> should be __BASE_SPARSE_MEM_HH__ src/base/sparse_mem.hh <http://reviews.m5sim.org/r/473/#comment1205> Probably should include "base/types.hh" instead src/base/sparse_mem.hh <http://reviews.m5sim.org/r/473/#comment1206> Brace on new line. src/base/sparse_mem.hh <http://reviews.m5sim.org/r/473/#comment1207> Non-standard use of spaces src/base/sparse_mem.hh <http://reviews.m5sim.org/r/473/#comment1209> why is this virtual? Are you expecting to use polymorphism? src/base/sparse_mem.hh <http://reviews.m5sim.org/r/473/#comment1208> excess whitespace. src/base/sparse_mem.cc <http://reviews.m5sim.org/r/473/#comment1210> should be "base/sparse_mem.hh" also this should go below system includes. src/base/sparse_mem.cc <http://reviews.m5sim.org/r/473/#comment1211> why not just return early here so you don't have to have such a huge else block? - Nathan 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
