MEM: Simplify cache ports preparing for master/slave split

Information
Submitter:	Andreas Hansson
Repository:	gem5
Branch:	default
Bugs:
Depends On:
Reviewers
Groups:	Default
People:

Description

MEM: Simplify cache ports preparing for master/slave split

This patch splits the two cache ports into a master (memory-side) and
slave (cpu-side) subclass of port with slightly different
functionality. For example, it is only the CPU-side port that blocks
incoming requests, and only the memory-side port that schedules send
events outside of what the transmit list dictates.

This patch simplifies the two classes by relying further on
SimpleTimingPort and also generalises the latter to better accommodate
the changes (introducing trySendTiming and scheduleSend). The
memory-side cache port overrides sendDeferredPacket to be able to not
only send responses from the transmit list, but also send requests
based on the MSHRs.

A follow on patch further simplifies the SimpleTimingPort and the
cache ports.

Testing Done

util/regress all passing (disregarding t1000 and eio)

Issue Summary

Description	From	Last Updated	Status
"not" should be "note", might want to add that we don't have a functionalSnoop method because functional requests do the ...	Steve Reinhardt	Feb. 22, 2012, 3:24 a.m.	Resolved
The default for label should be something generic but meaningful, like "SimpleTimingPort"	Steve Reinhardt	Feb. 22, 2012, 3:24 a.m.	Resolved
Does having a constructor parameter with the same name as a member even work? I'm surprised if it does...	Steve Reinhardt	Feb. 22, 2012, 3:26 a.m.	Resolved
I think this comment is a little misleading... I'd say "if we are waiting on a retry, there's no need ...	Steve Reinhardt	Feb. 22, 2012, 3:28 a.m.	Resolved
Missing part of this comment	Steve Reinhardt	Feb. 22, 2012, 3:28 a.m.	Resolved
Should "not" be "note" here? Also should "do not" be "may not"?	Steve Reinhardt	Feb. 22, 2012, 3:30 a.m.	Resolved

looks fine to me, but steve probably needs to take a look.

Most of my issues are with the comments and not with the code, so that's a pretty good sign...

src/mem/cache/cache_impl.hh (Diff revision 1)

"not" should be "note", might want to add that we don't have a functionalSnoop method because functional requests do the same thing regardless of which side they come in

Show all issues

src/mem/tport.hh (Diff revision 1)

The default for label should be something generic but meaningful, like "SimpleTimingPort"

Show all issues

src/mem/tport.cc (Diff revision 1)

Does having a constructor parameter with the same name as a member even work?  I'm surprised if it does...

Show all issues

Andreas Hansson Feb. 22, 2012, 3:30 a.m. (Feb. 22, 2012, 3:30 a.m.)

It works absolutely fine. Within this scope name refers to the parameter.

Steve Reinhardt Feb. 22, 2012, 4:06 a.m. (Feb. 22, 2012, 4:06 a.m.)

Interesting... learn something new every day.  I'm not surprised that the parameter declaration shadows the member, but I am surprised that the compiler doesn't give you a warning about that (as it does if you have a local var that shadows a param).  I'm also slightly surprised that "label(label)" works, since you've got the same name with two different bindings right there (though clearly the context is different so the compiler can distinguish them).  I always assumed that the idiom of naming either the param or the member something slightly different like "_label" was to avoid such problems, so it's mildly surprising to see that no such problems exist.

I guess I'd still like to see the param called "_label" just for consistency with all our other constructors.

Andreas Hansson Feb. 22, 2012, 7:38 a.m. (Feb. 22, 2012, 7:38 a.m.)

I'm happy to change it. (In my Strostrup-influenced C++ schooling we were encouraged to name the parameter and member identically if there is a 1:1 relation. That said, I do see how it can cause confusion if people aren't used to it.)

src/mem/tport.cc (Diff revision 1)

I think this comment is a little misleading... I'd say "if we are waiting on a retry, there's no need to schedule sendEvent"

Show all issues

src/mem/tport.cc (Diff revision 1)

Missing part of this comment

Show all issues

src/mem/tport.cc (Diff revision 1)

Should "not" be "note" here?  Also should "do not" be "may not"?

Show all issues

Diff:

Revision 2 (+313 -251)

Show changes

	src/mem/tport.hh
	src/mem/tport.cc
	src/mem/cache/base.hh
	src/mem/cache/base.cc
	src/mem/cache/cache.hh
	src/mem/cache/cache_impl.hh

Diff:

Revision 3 (+313 -251)

Show changes

	src/mem/tport.hh
	src/mem/tport.cc
	src/mem/cache/base.hh
	src/mem/cache/base.cc
	src/mem/cache/cache.hh
	src/mem/cache/cache_impl.hh

Diff:

Revision 4 (+313 -251)

Show changes

	src/mem/tport.hh
	src/mem/tport.cc
	src/mem/cache/base.hh
	src/mem/cache/base.cc
	src/mem/cache/cache.hh
	src/mem/cache/cache_impl.hh

Ship It!

You have a pending review.

Review Board 2.0.15

This change has been marked as submitted.

Screenshots

Files

Issue Summary

Diff:

Diff:

Diff:

Status: Closed (submitted)