----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: http://reviews.m5sim.org/r/277/#review406 -----------------------------------------------------------
Ship it! Overall, I have two major comments. 1) You should get rid of print_str() (and probably print()) for that matter and implement operator<<. 2) You shouldn't be duplicating so many lines with the __PRETTY_FUNCTION__, __FILE__, __LINE__ stuff. I think it should be buried in a macro. In fact, if people like, we could make those available to *ALL* DPRINTFs and make turning them on and off another trace flag. In all honesty though, they tend not to be necessary if your trace strings are at all descriptive. src/cpu/testers/rubytest/CheckTable.cc <http://reviews.m5sim.org/r/277/#comment551> You don't need __PRETTY_FUNCTION__ and __FILE__/__LINE__. I'd say that one or the other should do. Also if you're going to do this a lot, we should have a new DPRINTF macro for it. src/mem/SConscript <http://reviews.m5sim.org/r/277/#comment550> I would emphatically argue *no* src/mem/protocol/MESI_CMP_directory-L1cache.sm <http://reviews.m5sim.org/r/277/#comment553> I agree that we should pass it through unmodified. Also, DPRINTF understands operator<<, and if you use %s, it can use any object with an operator<<, so I'd get rid of print_str() on the objects and just rename it operator<< src/mem/protocol/MESI_CMP_directory-L1cache.sm <http://reviews.m5sim.org/r/277/#comment554> I agree with Steve. Commented out code is a no-no. src/mem/protocol/MESI_CMP_directory-L2cache.sm <http://reviews.m5sim.org/r/277/#comment556> Can you consolidate these into a single expression that has some context? I guess that's not the point of this diff though, so I don't think it's required. src/mem/protocol/MESI_CMP_directory-L2cache.sm <http://reviews.m5sim.org/r/277/#comment557> Same here src/mem/protocol/RubySlicc_Exports.sm <http://reviews.m5sim.org/r/277/#comment558> We should probably change this to operator<<, then again, I am not quite clear how ruby uses this. src/mem/ruby/SConsopts <http://reviews.m5sim.org/r/277/#comment559> Good work! src/mem/ruby/buffers/MessageBuffer.cc <http://reviews.m5sim.org/r/277/#comment560> I'd rather see a new DPRINT Macro. src/mem/ruby/buffers/MessageBuffer.cc <http://reviews.m5sim.org/r/277/#comment561> Generally, this is a bad idea. src/mem/ruby/buffers/MessageBuffer.cc <http://reviews.m5sim.org/r/277/#comment562> you don't need to use c_str(), and in fact name() is automatically inserted into all DPRINTF statements. src/mem/ruby/buffers/MessageBuffer.cc <http://reviews.m5sim.org/r/277/#comment563> why is "message" in the argument list? Also, why this very complicated expression to get some message string? I suggest getting rid of print_str() and making operator<< work. src/mem/ruby/common/Address.hh <http://reviews.m5sim.org/r/277/#comment564> I'd honestly get rid of both print and print_str and just simply implement operator<< src/mem/ruby/common/Address.hh <http://reviews.m5sim.org/r/277/#comment565> Yes. That is exactly correct. src/mem/ruby/network/simple/PerfectSwitch.cc <http://reviews.m5sim.org/r/277/#comment566> Pretty strange that you keep having things like "m_switch_id" in the argument list. src/mem/ruby/storebuffer/storebuffer.cc <http://reviews.m5sim.org/r/277/#comment567> Instead of commenting these out, you could add a new Trace flag for these and get rid of the #ifdefs. Not sure if it's useful or not though. - Nathan On 2010-10-19 17:30:48, Nilay Vaish wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > http://reviews.m5sim.org/r/277/ > ----------------------------------------------------------- > > (Updated 2010-10-19 17:30:48) > > > Review request for Default and Ruby Reviewers. > > > Summary > ------- > > Ruby currently uses GEMS debug support with the enum character string map to > enable certain debug messages. Meanwhile, M5 has debug print support that > works with scons. Compiling the m5.fast binary, the M5 debug statements are > removed, but the Ruby ones are not unless RUBY_DEBUG is not defined. This > patch moves Ruby to M5's debug print support. > > > Diffs > ----- > > src/cpu/testers/rubytest/CheckTable.cc 956ac83b0a58 > src/mem/SConscript 956ac83b0a58 > src/mem/protocol/MESI_CMP_directory-L1cache.sm 956ac83b0a58 > src/mem/protocol/MESI_CMP_directory-L2cache.sm 956ac83b0a58 > src/mem/protocol/MESI_CMP_directory-dir.sm 956ac83b0a58 > src/mem/protocol/MI_example-cache.sm 956ac83b0a58 > src/mem/protocol/MI_example-dir.sm 956ac83b0a58 > src/mem/protocol/MOESI_CMP_directory-L1cache.sm 956ac83b0a58 > src/mem/protocol/MOESI_CMP_directory-L2cache.sm 956ac83b0a58 > src/mem/protocol/MOESI_CMP_directory-dir.sm 956ac83b0a58 > src/mem/protocol/MOESI_CMP_directory-perfectDir.sm 956ac83b0a58 > src/mem/protocol/MOESI_CMP_token-L1cache.sm 956ac83b0a58 > src/mem/protocol/MOESI_CMP_token-L2cache.sm 956ac83b0a58 > src/mem/protocol/MOESI_CMP_token-dir.sm 956ac83b0a58 > src/mem/protocol/MOESI_hammer-cache.sm 956ac83b0a58 > src/mem/protocol/MOESI_hammer-dir.sm 956ac83b0a58 > src/mem/protocol/RubySlicc_Exports.sm 956ac83b0a58 > src/mem/protocol/RubySlicc_MemControl.sm 956ac83b0a58 > src/mem/protocol/RubySlicc_Types.sm 956ac83b0a58 > src/mem/ruby/SConsopts 956ac83b0a58 > src/mem/ruby/buffers/MessageBuffer.cc 956ac83b0a58 > src/mem/ruby/common/Address.hh 956ac83b0a58 > src/mem/ruby/common/DataBlock.hh 956ac83b0a58 > src/mem/ruby/common/Debug.hh 956ac83b0a58 > src/mem/ruby/common/Debug.cc 956ac83b0a58 > src/mem/ruby/common/NetDest.hh 956ac83b0a58 > src/mem/ruby/common/NetDest.cc 956ac83b0a58 > src/mem/ruby/network/garnet/fixed-pipeline/NetworkInterface_d.cc > 956ac83b0a58 > src/mem/ruby/network/garnet/fixed-pipeline/Switch_d.cc 956ac83b0a58 > src/mem/ruby/network/garnet/flexible-pipeline/NetworkInterface.cc > 956ac83b0a58 > src/mem/ruby/network/garnet/flexible-pipeline/Router.cc 956ac83b0a58 > src/mem/ruby/network/simple/PerfectSwitch.cc 956ac83b0a58 > src/mem/ruby/network/simple/Throttle.cc 956ac83b0a58 > src/mem/ruby/network/simple/Topology.cc 956ac83b0a58 > src/mem/ruby/slicc_interface/Message.hh 956ac83b0a58 > src/mem/ruby/slicc_interface/NetworkMessage.hh 956ac83b0a58 > src/mem/ruby/storebuffer/storebuffer.cc 956ac83b0a58 > src/mem/ruby/system/CacheMemory.cc 956ac83b0a58 > src/mem/ruby/system/DirectoryMemory.cc 956ac83b0a58 > src/mem/ruby/system/SConscript 956ac83b0a58 > src/mem/ruby/system/SparseMemory.cc 956ac83b0a58 > src/mem/ruby/tester/RaceyPseudoThread.cc 956ac83b0a58 > src/mem/slicc/ast/FuncCallExprAST.py 956ac83b0a58 > src/mem/slicc/symbols/StateMachine.py 956ac83b0a58 > src/mem/slicc/symbols/Type.py 956ac83b0a58 > > Diff: http://reviews.m5sim.org/r/277/diff > > > Testing > ------- > > > Thanks, > > Nilay > > _______________________________________________ m5-dev mailing list [email protected] http://m5sim.org/mailman/listinfo/m5-dev
