> On Feb. 1, 2013, 2:09 a.m., Andreas Hansson wrote: > > This patch is going in a good direction, but at the moment I find the > > naming and typing rather confusing. (it also highlights that perhaps we > > should make Tick a class just like Cycles to enable us to specify an > > explicit constructor and avoid gcc being helpful with implicit casting)
I agree with you. I was some times feeling annoyed that gcc does not complain about one type getting casted to another. The only thing I feel uncomfortable about is the size of the resulting object. I agree that there will be just one data member in that class, and absolutely no virtual functions. My lack of compiler knowledge makes me uncomfortable. There is atleast one place in Ruby where I knowingly did not move from Time to Cycles because the code assumes that the size of the object that contains the Time field is not going to change. You might want to blame me for making such an assumption. > On Feb. 1, 2013, 2:09 a.m., Andreas Hansson wrote: > > src/mem/ruby/buffers/MessageBuffer.cc, line 402 > > <http://reviews.gem5.org/r/1683/diff/1/?file=33760#file33760line402> > > > > This looks odd to me. Get Cycles and then turn ticks to cycles? The naming is problem here. getDelayedCycles() is what was in use before and used to return Cycles. But now it returns Ticks instead. I will change it to getDelayedTicks() throughout. > On Feb. 1, 2013, 2:09 a.m., Andreas Hansson wrote: > > src/mem/ruby/network/garnet/fixed-pipeline/NetworkInterface_d.cc, line 177 > > <http://reviews.gem5.org/r/1683/diff/1/?file=33761#file33761line177> > > > > Is getTime not in Cycles? No, it is in ticks. > On Feb. 1, 2013, 2:09 a.m., Andreas Hansson wrote: > > src/mem/ruby/slicc_interface/RubyRequest.hh, line 101 > > <http://reviews.gem5.org/r/1683/diff/1/?file=33767#file33767line101> > > > > Seems unrelated. Agreed. Please let it go through. - Nilay ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: http://reviews.gem5.org/r/1683/#review3956 ----------------------------------------------------------- On Jan. 29, 2013, 8:31 p.m., Nilay Vaish wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > http://reviews.gem5.org/r/1683/ > ----------------------------------------------------------- > > (Updated Jan. 29, 2013, 8:31 p.m.) > > > Review request for Default. > > > Description > ------- > > Changeset 9500:5dbd52da6716 > --------------------------- > ruby: enable multiple clock domanins > This patch allows ruby to have multiple clock domains. As I understand > with this patch, controllers can have different frequencies. The entire > network needs to run at a single frequency. > > The idea is that with in an object, time is treated in terms of cycles. > But the messages that are passed from one entity to another should contain > the time in cycles. As of now, this is only true for the message buffers, > but not for the links in the network. As I understand the code, all the > entities in different networks (simple, garnet-fixed, garnet-flexible) should > be clocked at the same frequency. > > Another problem is that the directory controller has to operate at the same > frequency as the ruby system. This is because the memory controller does > not make use of the Message Buffer, and instead implements a buffer of its > own. So, it has no idea of the frequency at which the directory controller > is operating and uses ruby system's frequency for scheduling events. > > > Diffs > ----- > > src/mem/ruby/buffers/MessageBuffer.cc 5dca39cc7641 > src/mem/ruby/network/garnet/fixed-pipeline/NetworkInterface_d.cc > 5dca39cc7641 > src/mem/ruby/network/garnet/flexible-pipeline/NetworkInterface.cc > 5dca39cc7641 > src/mem/ruby/network/simple/Throttle.cc 5dca39cc7641 > src/mem/ruby/profiler/Profiler.cc 5dca39cc7641 > src/mem/ruby/slicc_interface/Message.hh 5dca39cc7641 > src/mem/ruby/slicc_interface/NetworkMessage.hh 5dca39cc7641 > src/mem/ruby/slicc_interface/RubyRequest.hh 5dca39cc7641 > src/mem/ruby/system/DMASequencer.cc 5dca39cc7641 > src/mem/ruby/system/RubyMemoryControl.cc 5dca39cc7641 > src/mem/ruby/system/Sequencer.cc 5dca39cc7641 > src/mem/ruby/system/System.hh 5dca39cc7641 > src/mem/ruby/system/WireBuffer.cc 5dca39cc7641 > src/mem/slicc/ast/EnqueueStatementAST.py 5dca39cc7641 > src/mem/slicc/symbols/Type.py 5dca39cc7641 > > Diff: http://reviews.gem5.org/r/1683/diff/ > > > Testing > ------- > > Regressions pass. Patch has been tested very lightly using the ruby random > tester. > > > Thanks, > > Nilay Vaish > > _______________________________________________ gem5-dev mailing list [email protected] http://m5sim.org/mailman/listinfo/gem5-dev
