Yes, it's good to have an exhaustive list of all the ways that code is broken ;-). Thanks!
On Mon, Mar 22, 2010 at 11:12 AM, Beckmann, Brad <[email protected]> wrote: > 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: [email protected] [mailto:[email protected]] 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 <[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 > > > _______________________________________________ > 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
