X86 ISA: Change definitions of locked instructions

Information
Submitter:	Nilay Vaish
Repository:	gem5
Branch:	default
Bugs:
Depends On:
Reviewers
Groups:	Default
People:

Description

X86 ISA: Change definitions of locked instructions
This patch is for changing the defintion of locked instructions. These
should behave as memory barriers and should be executed in a non speculative
fashion.

Testing Done

I think the definitions of IO instructions should change. Since IO instructions
also act as memory barriers, the ld and st microops should be replaced with ldstl
and stul.

Gabe Black Oct. 30, 2011, 11:38 a.m. (Oct. 30, 2011, 11:38 a.m.)

No, they shouldn't. If those instructions need to be memory barriers, they need to be memory barriers on their own. The locking microops have to be in load/store pairs which these are not. Also the load microop will fault if a store would fault which is also not the desired behavior.

src/arch/x86/isa/insts/general_purpose/input_output/general_io.py (Diff revision 1)

You're changing the behavior of these microops, and that's not ok because these are used to talk to devices. These are nonspeculative on purpose, and I'm pretty sure making them speculative breaks the O3 FS regression. Have you run the regressions?

Diff:

Revision 2 (+145 -139)

Show changes

	src/arch/x86/isa/insts/general_purpose/logical.py
	src/arch/x86/isa/insts/general_purpose/semaphores.py
	src/arch/x86/isa/insts/general_purpose/arithmetic/add_and_subtract.py
	src/arch/x86/isa/insts/general_purpose/arithmetic/increment_and_decrement.py
	src/arch/x86/isa/insts/general_purpose/compare_and_test/bit_test.py
	src/arch/x86/isa/insts/general_purpose/data_transfer/xchg.py
	src/arch/x86/isa/insts/general_purpose/input_output/general_io.py
	src/arch/x86/isa/insts/general_purpose/input_output/string_io.py
	1 more

You shouldn't change any of the places where ldstl or stul are used. They should have the properties they need implicitly, and regular loads/stores should take a parameter to set those optionally.

Gabe Black Oct. 30, 2011, 8:30 p.m. (Oct. 30, 2011, 8:30 p.m.)

You could make the default for nonSpec and memBar True for ldstl or stul and False for the other loads and stores. Then it would make sense to still take an override for that parameter as an argument, but the default would be the right thing to do almost all of the time and no macroop definitions would need to change. The problem is that forcing those properties to be specified the same way every time the microop is used is redundant, verbose, and cumbersome, and those are exactly the things that having the microcode assembler is supposed to fix. Be careful how you implement that, though, since I suspect some of the subtleties of python may cause trouble.

Diff:

Revision 3 (+74 -24)

Show changes

	src/arch/x86/isa/insts/general_purpose/input_output/general_io.py
	src/arch/x86/isa/insts/general_purpose/input_output/string_io.py
	src/arch/x86/isa/microops/ldstop.isa

I'm not super excited about the duplicated code, but it's not totally unnecessary and I don't want to be too much of an obstacle to you fixing your bug. Be sure to run the regressions and look at the output carefully, and then I'm ok with this (and follow up with regression stat updates, as necessary). If it bugs me too much I'll go back and see if things can be refactored a bit.

Nilay Vaish Nov. 1, 2011, 5:55 a.m. (Nov. 1, 2011, 5:55 a.m.)

Gabe, given the discussion that we had about the o3 cpu, it seems to me
that this patch does not need to be committed. Instead I am posting a new
patch for fence microop in x86.

You have a pending review.

Review Board 2.0.15

This change has been discarded.

Screenshots

Files

Diff:

Diff: