I did see your comments on the isReadWrite check and that is why I added the "Fix Me" comment. The problem is I did not originally add the line and I have no way of testing any changes to this functionality. I assume this is needed by the libruby interface and I'm expecting the folks at Wisconsin to fix this.
Brad > -----Original Message----- > From: m5-dev-boun...@m5sim.org [mailto:m5-dev-boun...@m5sim.org] On > Behalf Of Steve Reinhardt > Sent: Sunday, March 21, 2010 11:05 PM > To: M5 Developer List > Subject: Re: [m5-dev] changeset in m5: ruby: Ruby support for LLSC > > Comments below... > > On Sun, Mar 21, 2010 at 10:47 PM, Brad Beckmann <brad.beckm...@amd.com> > wrote: > > changeset 185ad61a4117 in /z/repo/m5 > > details: http://repo.m5sim.org/m5?cmd=changeset;node=185ad61a4117 > > description: > > ruby: Ruby support for LLSC > > > > diffstat: > > > > 4 files changed, 102 insertions(+), 20 deletions(-) > > src/mem/ruby/system/CacheMemory.cc | 17 +++++++++- > > src/mem/ruby/system/RubyPort.cc | 58 > +++++++++++++++++++++++++++++------- > > src/mem/ruby/system/SConscript | 2 + > > src/mem/ruby/system/Sequencer.cc | 45 ++++++++++++++++++++++----- > > > > diffs (215 lines): > > > [...] > > - type = RubyRequestType_RMW_Write; > > } else { > > - panic("Unsupported ruby packet type\n"); > > + if (pkt->isRead()) { > > + if (pkt->req->isInstFetch()) { > > + type = RubyRequestType_IFETCH; > > + } else { > > + type = RubyRequestType_LD; > > + } > > + } else if (pkt->isWrite()) { > > + type = RubyRequestType_ST; > > + } else if (pkt->isReadWrite()) { > > + // > > + // Fix me. Just because the packet is a read/write > request does not > > + // necessary mean it is a read-modify-write atomic > operation. > > + // > > + type = RubyRequestType_RMW_Write; > > + } else { > > + panic("Unsupported ruby packet type\n"); > > + } > > } > > Did you miss my comment about isReadWrite() on this patch? Or did you > decide not to do anything since it's no worse than before? Seems like > it's at least worth adding a comment for posterity, but better yet to > fix it now if it needs fixing. > > > + > > + g_system_ptr->getProfiler()- > >profileTransition("Seq", > > + m_version, > > + Address(request.paddr), > > + "", > > + "SC Fail", > > + "", > > + > RubyRequestType_to_string(request.type)); > > + > > I'm still not thrilled with the line-count explosion your "parameter > per line" formatting gives, especially when we're dedicating entire > lines to parameters like '""'. As far as parsing the call, there are > enough args to this function that it's opaque to me what it's supposed > to be doing regardless of how many lines it takes. > > Steve > _______________________________________________ > m5-dev mailing list > m5-dev@m5sim.org > http://m5sim.org/mailman/listinfo/m5-dev _______________________________________________ m5-dev mailing list m5-dev@m5sim.org http://m5sim.org/mailman/listinfo/m5-dev
