> On June 23, 2015, 3:38 p.m., Joel Hestness wrote: > > src/mem/ruby/network/simple/Throttle.hh, line 55 > > <http://reviews.gem5.org/r/2901/diff/2/?file=46726#file46726line55> > > > > I don't understand why this is inconsistent with the GarnetNetwork > > changes: Here, you've added a per-instance pointer to Ruby to achieve the > > same functionality as with the GarnetNetwork changes, but in the > > GarnetNetwork, you're accessing the RubySystem from the params() in > > collateStats. > > > > I'd rather these be consistent. > > > > NOTE: If you were to follow my overall suggestion of making the > > start_time static in RubySystem, this change would not need to happen, and > > there would be no consistency issues between here and the GarnetNetwork. > > Brandon Potter wrote: > Throttle is not derived from a clocked memory object, but it does have a > clocked memory object that is passed into it. Is it reasonable to assume > that this clocked memory object will return the same cycle count as the > RubySystem object? > > If so, I think that it would be possible to avoid passing in the pointer > to RubySystem. If not, the RubySystem pointer needs to be passed in to give > the right cycle count with curCycle inside Throttle::collateStats(). > > Joel Hestness wrote: > Ok, I understand that you need to get the cycle count from somewhere and > that you've chosen the RubySystem for that. What I'm confused about is the > inconsistency of whether you add a member variable to point to the RubySystem > or access it from the params. Common gem5 convention is to add a member > variable any time that a param variable is used outside the constructor. Your > changes to Throttle follow this convention. I think it would be more > consistent to add member variables that point to the RubySystem from the > GarnetNetwork objects. > > Brandon Potter wrote: > Is it acceptable to leave the access through params() as is? It does > exactly the same thing, except the access is done through the params() field > instead of explicitly adding another member to the object. Feel like this > could be argued for in both ways, but the difference for me is just the extra > work in making the modifications. > > Joel Hestness wrote: > Fine, though I feel that consistency on small things like this should be > considered before submitting a patch for review (e.g. your g_system_ptr patch > has numerous inconsistencies like this as well). Substantial refactorings > like these are opportunities to make code more consistent and easier to > change in the future.
I have addressed all of your comments except this one. Personally, I feel that this one is a bit arbitrary given that the params are recorded in the class (what other purpose do they serve if the params are explicitly retained other than to use them?), but I will change it if you really want me to do so. I think the end result of your suggestions is that the code does look better and is a bit cleaner, but I disagree with this one case. Just to be thorough, what's bothering you about the g_system_ptr patch? - Brandon ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: http://reviews.gem5.org/r/2901/#review6573 ----------------------------------------------------------- On June 26, 2015, 5:34 p.m., Brandon Potter wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > http://reviews.gem5.org/r/2901/ > ----------------------------------------------------------- > > (Updated June 26, 2015, 5:34 p.m.) > > > Review request for Default. > > > Repository: gem5 > > > Description > ------- > > Changeset 10876:8f73cf051cdd > --------------------------- > ruby: replace global cycle counter w/ cycle per ruby system > > > Diffs > ----- > > src/mem/ruby/common/Global.hh ebb3d0737aa72ec4fa24b6af9cf9a6b2a1109d18 > src/mem/ruby/common/Global.cc ebb3d0737aa72ec4fa24b6af9cf9a6b2a1109d18 > src/mem/ruby/network/Network.cc ebb3d0737aa72ec4fa24b6af9cf9a6b2a1109d18 > src/mem/ruby/network/garnet/fixed-pipeline/GarnetNetwork_d.cc > ebb3d0737aa72ec4fa24b6af9cf9a6b2a1109d18 > src/mem/ruby/network/garnet/flexible-pipeline/GarnetNetwork.cc > ebb3d0737aa72ec4fa24b6af9cf9a6b2a1109d18 > src/mem/ruby/network/simple/Switch.cc > ebb3d0737aa72ec4fa24b6af9cf9a6b2a1109d18 > src/mem/ruby/network/simple/Throttle.hh > ebb3d0737aa72ec4fa24b6af9cf9a6b2a1109d18 > src/mem/ruby/network/simple/Throttle.cc > ebb3d0737aa72ec4fa24b6af9cf9a6b2a1109d18 > src/mem/ruby/system/System.hh ebb3d0737aa72ec4fa24b6af9cf9a6b2a1109d18 > src/mem/ruby/system/System.cc ebb3d0737aa72ec4fa24b6af9cf9a6b2a1109d18 > > Diff: http://reviews.gem5.org/r/2901/diff/ > > > Testing > ------- > > > Thanks, > > Brandon Potter > > _______________________________________________ gem5-dev mailing list [email protected] http://m5sim.org/mailman/listinfo/gem5-dev
