Hi Tony, Thank you for clarifying. Unfortunately, I feel that this is an inappropriate solution. Prior to changing the DMASequencer back to descending from RubyPort, none of the sequencers/coalescers needed to send range changes, because none are ever used to connect to address-routed components. With this change, the prior types will now all send unnecessary range updates, when only the DMASequencer should do this (it is the only that can/should be connected to address-routed components). I can understand that you didn't intend for your change to cause this, but it effectively changes the interface of sequencers/coalescers.
I feel strongly that this should be handled a different way, because this is the sort of change that has gotten us into our terrible sequencer/coalescer inheritance mess. Namely, there is poor delineation/inheritance among the existing types, which causes poorer derivative changes that further blur the lines/interfaces. I would recommend that you make the slave_ports visible to inherited types and just send the range change from DMASequencer::init(). This shouldn't cause any code duplication, as you suggest. Thank you, Joel On Wed, Feb 17, 2016 at 11:22 AM, Gutierrez, Anthony < [email protected]> wrote: > The sequencer originally did this because it maintained its own slave > port, because it wasn't derived from RubyPort. Now that it is, it no longer > has its own slave port, instead using RubyPorts slave_ports vector. > RubyPort didn't send the ranges for the slave_ports, and it is private, so > any derived class, e.g., DMASequencer, cannot do it in its own init() > function. > > By inspecting the code, it seemed that no derived classes of RubyPort > utilized the slave_ports vector in the RubyPort base class, which is why > this assert isn't being hit previously. > > Also, in general we'd like to keep common functionality in the base > RubyPort to avoid code duplication. If I made slave_ports protected, I > could send the range change via the init() call in the derived classes, but > there really is no point in doing that as it would be pure code dupe. > > -----Original Message----- > From: gem5-dev [mailto:[email protected]] On Behalf Of Joel > Hestness > Sent: Wednesday, February 17, 2016 9:06 AM > To: gem5 Developer List <[email protected]> > Cc: [email protected] > Subject: Re: [gem5-dev] changeset in gem5: ruby: send address ranges from > RubyPort > > Hi Tony, > Thanks for taking a look at the regression problem. I'm a little > confused about this fix though: The sendRangeChange() call was originally > in the DMASequencer, but not in the RubyPort. Here, you've added it in the > RubyPort. Shouldn't this have been put back into DMASequencer::init() > instead? > > Thanks! > Joel > > > On Wed, Feb 17, 2016 at 10:32 AM, Tony Gutierrez < > [email protected]> > wrote: > > > changeset e777659dcff6 in /z/repo/gem5 > > details: http://repo.gem5.org/gem5?cmd=changeset;node=e777659dcff6 > > description: > > ruby: send address ranges from RubyPort > > > > diffstat: > > > > src/mem/ruby/system/RubyPort.cc | 3 +++ > > 1 files changed, 3 insertions(+), 0 deletions(-) > > > > diffs (13 lines): > > > > diff -r a4d19e7cd26d -r e777659dcff6 src/mem/ruby/system/RubyPort.cc > > --- a/src/mem/ruby/system/RubyPort.cc Wed Feb 17 03:56:20 2016 -0500 > > +++ b/src/mem/ruby/system/RubyPort.cc Wed Feb 17 11:31:54 2016 -0500 > > @@ -84,6 +84,9 @@ > > { > > assert(m_controller != NULL); > > m_mandatory_q_ptr = m_controller->getMandatoryQueue(); > > + > > + for (const auto &s_port : slave_ports) > > + s_port->sendRangeChange(); > > } > > > > BaseMasterPort & > > _______________________________________________ > > gem5-dev mailing list > > [email protected] > > http://m5sim.org/mailman/listinfo/gem5-dev > > > > > > -- > Joel Hestness > PhD Candidate, Computer Architecture > Dept. of Computer Science, University of Wisconsin - Madison > http://pages.cs.wisc.edu/~hestness/ > _______________________________________________ > gem5-dev mailing list > [email protected] > http://m5sim.org/mailman/listinfo/gem5-dev > _______________________________________________ > gem5-dev mailing list > [email protected] > http://m5sim.org/mailman/listinfo/gem5-dev > -- Joel Hestness PhD Candidate, Computer Architecture Dept. of Computer Science, University of Wisconsin - Madison http://pages.cs.wisc.edu/~hestness/ _______________________________________________ gem5-dev mailing list [email protected] http://m5sim.org/mailman/listinfo/gem5-dev
