I see. I have limited understanding of that code, so I was trying to fix the error and really didn't know the intention of the code. I guess what should have happened is that the DMASequencer should have simply kept its slave port, and initialized it the way it always had. I can make that change.
-----Original Message----- From: gem5-dev [mailto:[email protected]] On Behalf Of Joel Hestness Sent: Wednesday, February 17, 2016 10:45 AM To: gem5 Developer List <[email protected]> Subject: Re: [gem5-dev] changeset in gem5: ruby: send address ranges from RubyPort Sorry for piling on, but here's a little more: Arguably, what should happen is that the DMASequencer should have its own address-routing-capable slave port separate from the standard slave ports of the RubyPort. I suspect this is a major reason why Nilay had previously disconnected the inheritance. Joel On Wed, Feb 17, 2016 at 12:42 PM, Joel Hestness <[email protected]> wrote: > 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/ > -- 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
