On Thu, Mar 18, 2010 at 3:46 PM, Brad Beckmann <[email protected]> wrote:
> diff --git a/src/mem/ruby/system/RubyPort.cc b/src/mem/ruby/system/RubyPort.cc
> --- a/src/mem/ruby/system/RubyPort.cc
> +++ b/src/mem/ruby/system/RubyPort.cc
> @@ -210,20 +210,31 @@
> pc = pkt->req->getPC();
> }
>
> - if (pkt->isRead()) {
> - if (pkt->req->isInstFetch()) {
> - type = RubyRequestType_IFETCH;
> + if (pkt->isLLSC()) {
> + if (pkt->isWrite()) {
> + DPRINTF(MemoryAccess, "Issuing SC\n");
> + type = RubyRequestType_Locked_Write;
> } else {
> - type = RubyRequestType_LD;
> + DPRINTF(MemoryAccess, "Issuing LL\n");
> + assert(pkt->isRead());
> + type = RubyRequestType_Locked_Read;
> }
> - } else if (pkt->isWrite()) {
> - type = RubyRequestType_ST;
> - } else if (pkt->isReadWrite()) {
> - 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()) {
> + type = RubyRequestType_RMW_Write;
> + } else {
> + panic("Unsupported ruby packet type\n");
> + }
> }
>From packet.hh:
bool isReadWrite() const { return isRead() && isWrite(); }
... so I don't think these if/else if chains are doing what you intend
:-). Looks like that's a problem that you inherited though.
> -
> +
> RubyRequest ruby_request(pkt->getAddr(),
> pkt->getPtr<uint8_t>(),
> pkt->getSize(),
> @@ -231,13 +242,22 @@
> type,
> RubyAccessMode_Supervisor,
> pkt);
> -
> +
> // Submit the ruby request
> RequestStatus requestStatus = ruby_port->makeRequest(ruby_request);
> - if (requestStatus == RequestStatus_Issued) {
> + if ((requestStatus == RequestStatus_Issued) ||
> + (requestStatus == RequestStatus_LlscFailed)) {
> + if (pkt->isLLSC() && pkt->isWrite()) {
> + if (requestStatus == RequestStatus_LlscFailed) {
> + DPRINTF(MemoryAccess, "SC failed\n");
> + pkt->req->setExtraData(0);
> + } else {
> + pkt->req->setExtraData(1);
> + }
> + }
Does RequestStatus_Issued mean that the request has been completed?
If not, does it make sense to call setExtraData? If yes, that's a
confusing name :-).
This leans more toward personal preference, but I'd replace that inner
if/else with:
int extra_data = (requestStatus == RequestStatus_LlscFailed) ? 1 : 0;
DPRINTF(MemoryAccess, "SC returns %d\n", extra_data);
pkt->req->setExtraData(extra_data);
I think that makes it clearer that you're unconditionally calling
setExtraData(), and the status field only influences the value you're
using. Plus it cuts down on the if nesting, which is starting to get
a little out of hand here.
> --- a/src/mem/ruby/system/SConscript
> +++ b/src/mem/ruby/system/SConscript
> @@ -49,3 +49,5 @@
> Source('Sequencer.cc', Werror=False)
> Source('System.cc')
> Source('TimerTable.cc')
> +
> +TraceFlag('RubyCache')
There's already an 'LLSC' trace flag defined that may be more
appropriate for some of these DPRINTFs.
> } else {
> - data.setData(ruby_request.data, request_address.getOffset(),
> ruby_request.len);
> +
> + data.setData(ruby_request.data,
> + request_address.getOffset(),
> + ruby_request.len);
> +
> }
Here's another style thing we should discuss... I'm glad you wrapped
the too-long line, but I'd really prefer to keep the number of lines
to a reasonable minimum, which means just splitting this call across
two lines instead of three. I've seen other places (for example,
immediately below) where you've added new calls with lots of params
and put each argument on a separate line, so I know that's your
preference, but it can have a seriously negative impact on the density
of code you can fit on a screen.
> + g_system_ptr->getProfiler()->profileTransition("Seq",
> + m_version,
> + Address(request.paddr),
> + "",
> + "SC Fail",
> + "",
> +
> RubyRequestType_to_string(request.type));
See above... I think this call to profileTransition could probably fit
on 3 lines (4 at the most) instead of 7.
Steve
_______________________________________________
m5-dev mailing list
[email protected]
http://m5sim.org/mailman/listinfo/m5-dev
