cpu: Add store-access operations

Information
Submitter:	Tony Gutierrez
Repository:	gem5
Branch:	default
Bugs:
Depends On:
Reviewers
Groups:	Default
People:

Description

Changeset 10818:d8ed395159dc
---------------------------
cpu: Add store-access operations

This patch was created by Bihn Pham during his internship at AMD.

This patch adds decoupled store operations that are commonly used by
high-performance processors. The patch modifies the O3 CPU model to issue
StoreAccess operations as soon as possible to acquire exclusive permission for
eventual stores. Later data store operations behave as normal.

In the classic memory model, StoreAccess requests are treated as HardPFExReq
and share the existing prefetch queue. Therefore, they affect performance only
if the prefetcher is enabled in the classic model.

In Ruby, StoreAccess requests are treated as RubyStore with NULL data field.

Testing Done

Issue Summary

Description	From	Last Updated	Status
I am not a huge fan of the massive conditional and the code flow. First simple change is to check ...	Stephan Diestelhorst	May 14, 2015, 9:11 a.m.	Open
What is the point of factoring this out into a separate function? I'd think most of the logic in LSQUnit::write ...	Stephan Diestelhorst	May 14, 2015, 9:11 a.m.	Open
Why is the store access not an invalidate?	Stephan Diestelhorst	May 14, 2015, 9:11 a.m.	Open
Superfluous debug? Should be handled by 302 already.	Stephan Diestelhorst	May 14, 2015, 9:11 a.m.	Open
Drop.	Stephan Diestelhorst	May 14, 2015, 9:11 a.m.	Open
Unneccessary fast-out (althrough I would like the fast-path to be more obvious in gernal in this function). Also.. who deallocates ...	Stephan Diestelhorst	May 14, 2015, 9:11 a.m.	Open
Please don't remove these assertions. Add a more specific one with a justification.	Stephan Diestelhorst	May 14, 2015, 9:11 a.m.	Open
Surperfluous (..) around isHWPrefetch vs quite (too IMHO) sparing usage of () in other introduced conditionals.	Stephan Diestelhorst	May 14, 2015, 9:11 a.m.	Open
pull out blk_addr computation	Stephan Diestelhorst	May 14, 2015, 9:11 a.m.	Open
Why add both and not start out with sending HardPFExReq straight away?	Stephan Diestelhorst	May 14, 2015, 9:11 a.m.	Open
Drop, if not used anywhere except redundant check which could be isInvalidate.	Stephan Diestelhorst	May 14, 2015, 9:11 a.m.	Open
The comment is confusing; the "hence" part. Why isn't the HardPFexReq marked as NeedsResponse, too (similar to HardPFReq)?	Stephan Diestelhorst	May 14, 2015, 9:11 a.m.	Open
See earlier comment regarding NeedsResponse?	Stephan Diestelhorst	May 14, 2015, 9:11 a.m.	Open

Stephan Diestelhorst May 14, 2015, 9:12 a.m. (May 14, 2015, 9:12 a.m.)

Great idea, some comments on style and adding too many flags and specialisations.

src/cpu/o3/lsq_unit.hh (Diff revision 1)

I am not a huge fan of the massive conditional and the code flow.  First simple change is to check for the inverse and return NoFault early.
Is there any chance to annotate the reasoning behind some of these?

Show all issues

src/cpu/o3/lsq_unit_impl.hh (Diff revision 1)

What is the point of factoring this out into a separate function? I'd think most of the logic in LSQUnit::write could / should be moved into this function. Wrapping the dcachePort->sendTimingReq really does not make any difference.

Show all issues

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

Why is the store access not an invalidate?

Show all issues

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

Superfluous debug? Should be handled by 302 already.

Show all issues

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

Drop.

Show all issues

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

Unneccessary fast-out (althrough I would like the fast-path to be more obvious in gernal in this function).

Also.. who deallocates the pkt?

Show all issues

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

Please don't remove these assertions. Add a more specific one with a justification.

Show all issues

src/mem/cache/mshr.cc (Diff revision 1)

Surperfluous (..) around isHWPrefetch vs quite (too IMHO) sparing usage of () in other introduced conditionals.

Show all issues

src/mem/cache/prefetch/queued.cc (Diff revision 1)

pull out blk_addr computation

Show all issues

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

Why add both and not start out with sending HardPFExReq straight away?

Show all issues

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

Drop, if not used anywhere except redundant check which could be isInvalidate.

Show all issues

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

The comment is confusing; the "hence" part. Why isn't the HardPFexReq marked as NeedsResponse, too (similar to HardPFReq)?

Show all issues

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

See earlier comment regarding NeedsResponse?

Show all issues

Description:

~		Changeset 10845:0aa7c6802baa
	~	Changeset 10818:d8ed395159dc

		cpu: Add store-access operations

		This patch was created by Bihn Pham during his internship at AMD.

		This patch adds decoupled store operations that are commonly used by
		high-performance processors. The patch modifies the O3 CPU model to issue
		StoreAccess operations as soon as possible to acquire exclusive permission for
		eventual stores. Later data store operations behave as normal.

		In the classic memory model, StoreAccess requests are treated as HardPFExReq
		and share the existing prefetch queue. Therefore, they affect performance only
		if the prefetcher is enabled in the classic model.

		In Ruby, StoreAccess requests are treated as RubyStore with NULL data field.

Diff:

Revision 2 (+229 -17)

Show changes

	src/cpu/o3/lsq_unit.hh
	src/cpu/o3/lsq_unit_impl.hh
	src/mem/abstract_mem.cc
	src/mem/packet.hh
	src/mem/packet.cc
	src/mem/cache/cache_impl.hh
	src/mem/cache/mshr.cc
	src/mem/cache/prefetch/queued.cc
	4 more

You have a pending review.

Review Board 2.0.15

Screenshots

Files

Issue Summary

Description:

Changeset 10845:0aa7c6802baa

Changeset 10818:d8ed395159dc

Diff: