riscv: [Patch 7/5] Corrected LRSC semantics

Information
Submitter:	Alec Roelke
Repository:	gem5
Branch:	default
Bugs:
Depends On:
Reviewers
Groups:	Default
People:

Description

Changeset 11694:d650c7e96464
---------------------------
riscv: [Patch 7/5] Corrected LRSC semantics

RISC-V makes use of load-reserved and store-conditional instructions to
enable creation of lock-free concurrent data manipulation as well as
ACQUIRE and RELEASE semantics for memory ordering of LR, SC, and AMO
instructions (the latter of which do not follow LR/SC semantics). This
patch is a correction to patch 4, which added these instructions to the
implementation of RISC-V. It modifies locked_mem.hh and the
implementations of lr.w, sc.w, lr.d, and sc.d to apply the proper gem5
flags and return the proper values.

An important difference between gem5's LLSC semantics and RISC-V's LR/SC
ones, beyond the name, is that gem5 uses 0 to indicate failure and 1 to
indicate success, while RISC-V is the opposite. Strictly speaking, RISC-V
uses 0 to indicate success and nonzero to indicate failure where the
value would indicate the error, but currently only 1 is reserved as a
failure code by the ISA reference.

This is the seventh patch in the series which originally consisted of five
patches that added the RISC-V ISA to gem5. The original five patches added
all of the instructions and added support for more detailed CPU models and
the sixth patch corrected the implementations of Linux constants and
structs. There will be an eighth patch that adds some regression tests
for the instructions.

[Removed some commented-out code from locked_mem.hh.]
Signed-off by: Alec Roelke

Testing Done

Issue Summary

Description	From	Last Updated	Status
remove commented out code	Tony Gutierrez	Nov. 20, 2016, 11:51 a.m.	Resolved
If you move this above the dprintf, you can use it to print the address in the dprint.	Tony Gutierrez	Nov. 20, 2016, 11:51 a.m.	Resolved
Should you be using cacheBlockMask here, as opposed to 0xF?	Tony Gutierrez	Nov. 20, 2016, 12:07 p.m.	Dropped

This doesn't work with the O3 CPU model; I wrote a simple program that performs a lr.w followed by sc.w that works with the atomic-simple, timing-simple, and minor CPU models, but with the O3 model I get this error:
gem5.debug: build/RISCV/mem/cache/cache.cc\:162: void Cache::satisfyRequest(PacketPtr, CacheBlk*, bool, bool): Assertion `pkt->getOffset(blkSize) + pkt->getSize() <= blkSize' failed.
I can't seem to track down what's causing the error.  Can anybody help me?

Ali Saidi Nov. 2, 2016, 3:28 p.m. (Nov. 2, 2016, 3:28 p.m.)

the issue is that the ld + st crosses a cache line which the O3 doesn't support doing this type of op when it spans two cache lines because you have to track both of them.

Alec Roelke Nov. 2, 2016, 6:27 p.m. (Nov. 2, 2016, 6:27 p.m.)

I see. It sounds like fixing it would require making changes to the O3 model itself, is that right? Are there changes I can make within the scope of this patch, or others in the series, that will make it work? This may actually explain other errors I'm getting with the O3 model with some of the other tests I'm making.

Tony Gutierrez Nov. 10, 2016, 8:48 a.m. (Nov. 10, 2016, 8:48 a.m.)

I'm not sure if you would need to necessarily modify the O3 model much, if at all. Certainly you will need to have your memory instructions detect an access that will cross a cache line boundary, split it into two, and track both. Then you will only consider it completed when both reqs get responses. This is done in a few other places in the model.

Alec Roelke Nov. 20, 2016, noon (Nov. 20, 2016, noon)

Can you point me to where this is done? I don't see it in any of the ISAs I checked. From what I can tell, it should be easy enough to split a memory request into two, and for readMemAtomic reconstructing the results should be fairly easy, but even if calling readMemTiming twice in a single LoadInitiateAcc worked and created two corresponding LoadCompleteAccs, it doesn't seem to be possible to reconstruct the data that was read from those two LoadCompleteAccs. Would I have to use micro-ops?

Tony Gutierrez Nov. 21, 2016, 9:19 a.m. (Nov. 21, 2016, 9:19 a.m.)

Take a look at the splitRequest() methods in the BaseDynInst class and how this interacts with the O3 model, I think it is a good starting point.

src/arch/riscv/locked_mem.hh (Diff revision 1)

remove commented out code

Show all issues

src/arch/riscv/locked_mem.hh (Diff revision 1)

If you move this above the dprintf, you can use it to print the address in the dprint.

Show all issues

src/arch/riscv/locked_mem.hh (Diff revision 1)

Should you be using cacheBlockMask here, as opposed to 0xF?

Show all issues

Alec Roelke Nov. 15, 2016, 7:48 p.m. (Nov. 15, 2016, 7:48 p.m.)

That would make more sense to me, also, but cacheBlockMask isn't a parameter for handleLockedRead. In MIPS, which this is based off of, ~0xF is used here as well (and in handleLockedWrite even though handleLockedWrite does have a cacheBlockMask parameter).

Description:

~		Changeset 11694:1c3068cb5c86
	~	Changeset 11694:d650c7e96464

		riscv: [Patch 7/5] Corrected LRSC semantics

		RISC-V makes use of load-reserved and store-conditional instructions to
		enable creation of lock-free concurrent data manipulation as well as
		ACQUIRE and RELEASE semantics for memory ordering of LR, SC, and AMO
		instructions (the latter of which do not follow LR/SC semantics). This
		patch is a correction to patch 4, which added these instructions to the
		implementation of RISC-V. It modifies locked_mem.hh and the
		implementations of lr.w, sc.w, lr.d, and sc.d to apply the proper gem5
		flags and return the proper values.

		An important difference between gem5's LLSC semantics and RISC-V's LR/SC
		ones, beyond the name, is that gem5 uses 0 to indicate failure and 1 to
		indicate success, while RISC-V is the opposite. Strictly speaking, RISC-V
		uses 0 to indicate success and nonzero to indicate failure where the
		value would indicate the error, but currently only 1 is reserved as a
		failure code by the ISA reference.

		This is the seventh patch in the series which originally consisted of five
		patches that added the RISC-V ISA to gem5. The original five patches added
		all of the instructions and added support for more detailed CPU models and
		the sixth patch corrected the implementations of Linux constants and
		structs. There will be an eighth patch that adds some regression tests
		for the instructions.

~		Signed-off by: Alec Roelke
	~	[Removed some commented-out code from locked_mem.hh.]
	+	Signed-off by: Alec Roelke

Diff:

Revision 2 (+56 -62)

Show changes

	src/arch/riscv/locked_mem.hh
	src/arch/riscv/isa/decoder.isa
	src/arch/riscv/isa/formats/mem.isa

You have a pending review.

Review Board 2.0.15

This change has been marked as submitted.

Screenshots

Files

Issue Summary

Description:

Changeset 11694:1c3068cb5c86

Changeset 11694:d650c7e96464

Diff:

Status: Closed (submitted)