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 <[email protected]> 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: [email protected] [mailto:[email protected]] 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 <[email protected]> >> 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 >> [email protected] >> http://m5sim.org/mailman/listinfo/m5-dev > > > _______________________________________________ > m5-dev mailing list > [email protected] > http://m5sim.org/mailman/listinfo/m5-dev > _______________________________________________ m5-dev mailing list [email protected] http://m5sim.org/mailman/listinfo/m5-dev
