mem: add request types for acquire and release

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

Description

Changeset 11225:89acf0bcf147
---------------------------
mem: add request types for acquire and release

Add support for acquire and release requests.  These synchronization operations
are commonly supported by several modern instruction sets.

Testing Done

Issue Summary

Description	From	Last Updated	Status
Should not be needed. There is space.	Andreas Hansson	Nov. 19, 2015, 1:07 p.m.	Resolved
No command has both set. Seems odd to check...	Andreas Hansson	Nov. 19, 2015, 1:42 p.m.	Dropped
This seems strange to me. We rely on the flags being fixed. I don't see why you would want to ...	Andreas Hansson	Nov. 19, 2015, 2:48 p.m.	Resolved
If you make these 0x00020000 and 0x00040000 they could be consecutive. Could you please also elaborate on the description of ...	Andreas Hansson	Nov. 20, 2015, 12:01 p.m.	Resolved
This seems odd. No packet type has both set.	Andreas Hansson	Nov. 20, 2015, 12:01 p.m.	Dropped

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

Should not be needed. There is space.

Show all issues

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

No command has both set. Seems odd to check...

Show all issues

Tony Gutierrez Nov. 19, 2015, 2:47 p.m. (Nov. 19, 2015, 2:47 p.m.)

There are HSAIL instructions that have acq/rel semantics, although they aren't in this patch. There are uses of this in the big GPU patch.

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

This seems strange to me. We rely on the flags being fixed. I don't see why you would want to change such a fundamental assumption.

Show all issues

Tony Gutierrez Nov. 19, 2015, 2:49 p.m. (Nov. 19, 2015, 2:49 p.m.)
```
You're right. This is not needed.
```

Why are you changing the numerical values of existing flags? Doing that will break existing memory traces, which will cause a lot of trouble for people with existing traces (there are quite a few of them).

Description:

~		Changeset 11186:915989ed4b8a
	~	Changeset 11225:ece5b1d556ba

		mem: add request types for acquire and release

		Add support for acquire and release requests. These synchronization operations
		are commonly supported by several modern instruction sets.

Diff:

Revision 2 (+22 -5)

Show changes

	src/mem/packet.hh
	src/mem/packet.cc
	src/mem/request.hh

Description:

~		Changeset 11225:ece5b1d556ba
	~	Changeset 11225:58ad4ff321ee

		mem: add request types for acquire and release

		Add support for acquire and release requests. These synchronization operations
		are commonly supported by several modern instruction sets.

Diff:

Revision 3 (+20 -5)

Show changes

	src/mem/packet.hh
	src/mem/packet.cc
	src/mem/request.hh

Description:

~		Changeset 11225:58ad4ff321ee
	~	Changeset 11225:9e0fa0306f8b

		mem: add request types for acquire and release

		Add support for acquire and release requests. These synchronization operations
		are commonly supported by several modern instruction sets.

Diff:

Revision 4 (+21 -5)

Show changes

	src/mem/packet.hh
	src/mem/packet.cc
	src/mem/request.hh

Thanks! My concerns have been addressed, but please make sure to get an ack from the other Andreas as well.

src/mem/request.hh (Diff revision 4)

If you make these 0x00020000 and 0x00040000 they could be consecutive. Could you please also elaborate on the description of these flags, what promises/requirements are imposed etc.

Show all issues

src/mem/request.hh (Diff revision 4)

This seems odd. No packet type has both set.

Show all issues

I am still not really understanding the request and the packet part. How are the two related (are they)?

Tony Gutierrez Nov. 20, 2015, 12:04 p.m. (Nov. 20, 2015, 12:04 p.m.)

I have a patch to undo the packet ACQ/REL changes, and will post it soon. Honestly I think the original author of this patch may have only added the Command for ACQ/REL because they needed something to pass to the Packet() ctor and no other Commands fit, at least that is my best guess.

Andreas Hansson Nov. 21, 2015, 4:19 a.m. (Nov. 21, 2015, 4:19 a.m.)

What is the big picture here? They are added as request flags (although they are mutually exclusive with the other request flags)? They are not appearing as packets, but there will be read/write packets that have requests that are acquire/release? Please don't commit this until we have sorted out all this confusion.

Tony Gutierrez Nov. 21, 2015, 9:13 a.m. (Nov. 21, 2015, 9:13 a.m.)

Yes, they are mutually exclusive with other request flags, i.e., all code for ACQ/REQ requests in the memory system is unique to only requests with isAcquire() or isRelease(). After creating the patch to remove ACQ/REL stuff from the packet our code works ok with regular Read/Write Cmds, but I was thinking it would make sense to have something like SerializingReq or SynchronizingReq or FenceReq in the packet with its own unique attributes, which could potentially be used for all sync/serializing requests across ISAs. This could even potentially clean up some of this code, because as noted before the Command/Attribute/Request Flags interface isn't very clearly defined and everyone seems to have their own ideas as to what they mean.

Andreas Hansson Nov. 21, 2015, 9:15 a.m. (Nov. 21, 2015, 9:15 a.m.)

That sounds great. I agree that it is a bit of a mess at the moment, and finding one common abstraction for all the ISAs would be a massive step in the right direction. Will you post a proposal once you've got something working?

Description:

~		Changeset 11225:9e0fa0306f8b
	~	Changeset 11225:686de3fd198d

		mem: add request types for acquire and release

		Add support for acquire and release requests. These synchronization operations
		are commonly supported by several modern instruction sets.

Diff:

Revision 5 (+21 -4)

Show changes

	src/mem/packet.hh
	src/mem/packet.cc
	src/mem/request.hh

Description:

~		Changeset 11225:686de3fd198d
	~	Changeset 11225:89acf0bcf147

		mem: add request types for acquire and release

		Add support for acquire and release requests. These synchronization operations
		are commonly supported by several modern instruction sets.

Diff:

Revision 6 (+11)

Show changes

src/mem/request.hh

Ship It!

Andreas H., I want to make sure you're explicitly ok with me shipping this. Do we have a ship it from you? Thanks.

You have a pending review.

Review Board 2.0.15

This change has been marked as submitted.

Screenshots

Files

Issue Summary

Description:

Changeset 11186:915989ed4b8a

Changeset 11225:ece5b1d556ba

Diff:

Description:

Changeset 11225:ece5b1d556ba

Changeset 11225:58ad4ff321ee

Diff:

Description:

Changeset 11225:58ad4ff321ee

Changeset 11225:9e0fa0306f8b

Diff:

Description:

Changeset 11225:9e0fa0306f8b

Changeset 11225:686de3fd198d

Diff:

Description:

Changeset 11225:686de3fd198d

Changeset 11225:89acf0bcf147

Diff:

Status: Closed (submitted)