Yes, I only identified that the programmer's intent was ill specified, I should have also identified that the code never gets executed.
I'll check in a change now. Brad > -----Original Message----- > From: m5-dev-boun...@m5sim.org [mailto:m5-dev-boun...@m5sim.org] On > Behalf Of Steve Reinhardt > Sent: Monday, March 22, 2010 9:13 AM > To: M5 Developer List > Subject: Re: [m5-dev] changeset in m5: ruby: Ruby support for LLSC > > OK... it does need a "fix me" comment, but the text doesn't really > identify the issue; it should include something like "this code never > gets executed because isReadWrite() implies both isRead() and > isWrite() are also true". > > For the record, we'll need to move away from sending RMW operations to > the memory system to sending a locked read followed by a locked write > to support generic x86 locked operations anyway. > > Steve > > On Mon, Mar 22, 2010 at 9:03 AM, Beckmann, Brad <brad.beckm...@amd.com> > wrote: > > 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 > > > _______________________________________________ > 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
