----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: http://reviews.gem5.org/r/1095/#review2304 -----------------------------------------------------------
Thanks again for publishing this patch. It is a good start, but a lot of work remains before it is ready to be checked in. It appears that some of these changes are actual improvements to the simple network itself. It may be help to split those up into a separate patch. Please let me know if you have any questions. I'll be happy to take a look at your next rev of the patch when it is ready. TPZSimul.ini <http://reviews.gem5.org/r/1095/#comment2710> What files is this .ini file pointing to? They don't seem be included in this patch. Are they necessary? If so, they should be part of the patch. If not, then I don't understand the need for this file? Also can you move this file to a separate Topaz directory underneath ruby/network? build_opts/ALPHA_MOESI_CMP_directory <http://reviews.gem5.org/r/1095/#comment2711> I don't think we want to turn on Topaz by default for any protocol. build_opts/ALPHA_Network_test <http://reviews.gem5.org/r/1095/#comment2712> Unnecesary whitespace change. Please remove. configs/ruby/Ruby.py <http://reviews.gem5.org/r/1095/#comment2713> Please stick to 80 char limit. Also I prefer to see comments on separate lines. configs/ruby/Ruby.py <http://reviews.gem5.org/r/1095/#comment2715> Instead of setting each individual parameter here, please create a new Topaz Network model that inherrits from the SimpleNetwork and predefines this values. In general, Topaz need to be a peer network, similar to the Simple and Garnet networks. Not a hacked up version of the simple network. configs/ruby/Ruby.py <http://reviews.gem5.org/r/1095/#comment2714> unnecessary whitespace change src/mem/ruby/buffers/MessageBuffer.hh <http://reviews.gem5.org/r/1095/#comment2716> Do not use #ifdefs. src/mem/ruby/buffers/MessageBuffer.hh <http://reviews.gem5.org/r/1095/#comment2717> Why do you need these functions defined here? Pleaes explain. src/mem/ruby/network/garnet/BaseGarnetNetwork.hh <http://reviews.gem5.org/r/1095/#comment2718> I'm confused. I thought Topaz was based off the simple network. Why are you defining Topaz functions in this Garnet file? src/mem/ruby/network/simple/PerfectSwitch.hh <http://reviews.gem5.org/r/1095/#comment2719> Instead of using an ifdef here, can you make a separate TopazSwitch object that inherits from PerfectSwitch? I think that would be much cleaner. src/mem/ruby/network/simple/PerfectSwitch.cc <http://reviews.gem5.org/r/1095/#comment2720> Again, all this Topaz specific code should be moved to a separate TopazSwitch.cc file. src/mem/ruby/network/simple/PerfectSwitch.cc <http://reviews.gem5.org/r/1095/#comment2722> consistent spacing: "int c = 0;" not "int c=0;" src/mem/ruby/network/simple/PerfectSwitch.cc <http://reviews.gem5.org/r/1095/#comment2721> Please stick to 80 chars and the correct spacing rules. For example, no spaces on each side of "::". src/mem/ruby/network/simple/PerfectSwitch.cc <http://reviews.gem5.org/r/1095/#comment2724> shift left to line up with the rest of this function. src/mem/ruby/network/simple/PerfectSwitch.cc <http://reviews.gem5.org/r/1095/#comment2723> Don't delete these lines. They improve the readablity of this code. src/mem/ruby/network/simple/PerfectSwitch.cc <http://reviews.gem5.org/r/1095/#comment2725> Don't comment out lines...simply remove them instead. src/mem/ruby/network/simple/PerfectSwitch.cc <http://reviews.gem5.org/r/1095/#comment2726> Don't change function definitions. Please stick to the gem5 style guidelines. src/mem/ruby/network/simple/SimpleNetwork.hh <http://reviews.gem5.org/r/1095/#comment2728> Again, move to a new Topaz network file. src/mem/ruby/network/simple/SimpleNetwork.cc <http://reviews.gem5.org/r/1095/#comment2729> 80 char limit src/mem/ruby/network/simple/SimpleNetwork.cc <http://reviews.gem5.org/r/1095/#comment2730> Don't add commented out source code. src/mem/ruby/network/simple/SimpleNetwork.py <http://reviews.gem5.org/r/1095/#comment2727> Move to a new network oject called TopazNetwork. Stick to 80 char limit per line. src/mem/ruby/network/simple/Switch.cc <http://reviews.gem5.org/r/1095/#comment2731> Again, don't change style src/mem/ruby/system/MachineID.hh <http://reviews.gem5.org/r/1095/#comment2732> Don't add commented out source code lines. - Brad Beckmann On March 12, 2012, 10:32 a.m., Valentin Puente wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > http://reviews.gem5.org/r/1095/ > ----------------------------------------------------------- > > (Updated March 12, 2012, 10:32 a.m.) > > > Review request for Default, Nilay Vaish and Brad Beckmann. > > > Description > ------- > > Interface to integrate TOPAZ network simulator > (http://code.google.com/p/tpzsimul/) within GEM5-RUBY. Does not replace > ruby's interconnection network simulators (garnet and simple-network). It > just adds the hookups required to connect ruby and TOPAZ. You have to > download as "ext" TOPAZ from an external repo. > > To have a better perspective of the changes, you can follow the guide > provided in http://code.google.com/p/tpzsimul/wiki/GEM5Integration. In order > to integrate the compilation process of both simulators we have to slightly > modify main SConstruct and ruby SCons files. All the remaining changes are > constrained to Ruby. Main modifications have been done in > "src/mem/ruby/network/simple/PerfectSwitch.cc" > > > Diffs > ----- > > SConstruct be4990a2c764 > TPZSimul.ini PRE-CREATION > build_opts/ALPHA_MOESI_CMP_directory be4990a2c764 > build_opts/ALPHA_Network_Topaz_test PRE-CREATION > build_opts/ALPHA_Network_test be4990a2c764 > build_opts/ALPHA_directory_topaz PRE-CREATION > build_opts/ALPHA_token_topaz PRE-CREATION > configs/ruby/Ruby.py be4990a2c764 > src/mem/ruby/SConscript be4990a2c764 > src/mem/ruby/SConsopts be4990a2c764 > src/mem/ruby/buffers/MessageBuffer.hh be4990a2c764 > src/mem/ruby/buffers/MessageBuffer.cc be4990a2c764 > src/mem/ruby/network/Network.hh be4990a2c764 > src/mem/ruby/network/Topology.cc be4990a2c764 > src/mem/ruby/network/garnet/BaseGarnetNetwork.hh be4990a2c764 > src/mem/ruby/network/simple/PerfectSwitch.hh be4990a2c764 > src/mem/ruby/network/simple/PerfectSwitch.cc be4990a2c764 > src/mem/ruby/network/simple/SimpleNetwork.hh be4990a2c764 > src/mem/ruby/network/simple/SimpleNetwork.cc be4990a2c764 > src/mem/ruby/network/simple/SimpleNetwork.py be4990a2c764 > src/mem/ruby/network/simple/Switch.hh be4990a2c764 > src/mem/ruby/network/simple/Switch.cc be4990a2c764 > src/mem/ruby/profiler/Profiler.cc be4990a2c764 > src/mem/ruby/system/MachineID.hh be4990a2c764 > > Diff: http://reviews.gem5.org/r/1095/diff/ > > > Testing > ------- > > Regression test seems to be working for the ruby protocols provided > > > Thanks, > > Valentin Puente > > _______________________________________________ gem5-dev mailing list [email protected] http://m5sim.org/mailman/listinfo/gem5-dev
