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
