ARM/Alpha/Cpu: Change prefetchs to be more like normal loads.

Information
Submitter:	Ali Saidi
Repository:	gem5
Branch:
Bugs:
Depends On:
Reviewers
Groups:	Default
People:

Description

ARM/Alpha/Cpu: Change prefetchs to be more like normal loads.

This change modifies the way prefetches work. They are now like normal loads
that don't writeback a register. Previously prefetches were supposed to call
prefetch() on the exection context, so they executed with execute() methods
instead of initiateAcc() completeAcc(). The prefetch() methods for all the CPUs
are blank, meaning that they get executed, but don't actually do anything.

On Alpha dead cache copy code was removed and prefetches are now normal ops.
They count as executed operations, but still don't do anything and IsMemRef is
not longer set on them.

On ARM IsDataPrefetch or IsInstructionPreftech is now set on all prefetch
instructions. The timing simple CPU doesn't try to do anything special for
prefetches now and they execute with the normal memory code path.

Testing Done

src/cpu/simple/timing.cc (Diff revision 1)

This change marks prefetches as regular load ops so they can actually execute. Alpha prefetches don't do anything at the moment and don't have an initiateAcc() method, so there needs to be a change to the ISA description for them.

src/cpu/simple/timing.cc (Diff revision 1)

I don't think this is right. This will likely break X86_FS where curMacroStaticInst will almost always be set. If you're having trouble with this in ARM, a possible culprit is that you're not setting isDelayedCommit on microops that aren't at the end of a macroop.

Ali Saidi Oct. 10, 2010, 1:08 p.m. (Oct. 10, 2010, 1:08 p.m.)
```
I'll look at isDelayedCommit.
```

src/cpu/simple/timing.cc (Diff revision 1)

Prefetch faults should ideally be surpressed at their source, not here. At the source we already implicitly know the instruction shouldn't fault. Here, we have to rediscover that all the time.

Ali Saidi Oct. 10, 2010, 1:08 p.m. (Oct. 10, 2010, 1:08 p.m.)

I don't agree. If you suppress this at the source the CPU model doesn't know if it should actually try and issue the instruction, so that would have to be checked some how. Additionally, in most cases there is one or two place in the TLB that a correct translation is returned and as many places where a fault is returned. Checking there would complication all of the code paths.

Steve Reinhardt Oct. 18, 2010, 9:52 a.m. (Oct. 18, 2010, 9:52 a.m.)

I don't follow your argument... shouldn't the prefetch get issued unconditionally?  If it doesn't have a valid translation then you don't want to send it to the cache, but the instruction itself still gets executed.

As far as implementing this, seems like there could be just one or two places after the TLB translation is returned where we override the fault if it's a prefetch, rather than pushing the override into the TLB itself.

Ali Saidi Oct. 18, 2010, 11:53 a.m. (Oct. 18, 2010, 11:53 a.m.)

I think the prefetch should be sent the the TLB unconditionally, and then if the prefetch faults the CPU should toss the instruction rather than the TLB returning no fault and the CPU i guess checking if the PA is set? 

I agree that we should override the fault in the CPU. Are we violently agreeing?

src/cpu/simple/timing.cc (Diff revision 1)

I'm not sure this is necessary. If the CPU isn't running, that's because it's waiting for something. If it's waiting for something, then it's doing a memory access, and there won't be a fault. The original assert should be right.

src/cpu/simple/timing.cc (Diff revision 1)

I'll check it out.

src/arch/arm/faults.cc (Diff revision 1)

This looks a bug fix that's unrelated to prefetching and should go in a separate patch.

src/cpu/simple/timing.cc (Diff revision 1)

yea, I don't get this either

src/cpu/simple/timing.cc (Diff revision 1)

I agree, prefetches should just never set fault to anything other than NoFault.

src/cpu/simple/timing.cc (Diff revision 1)

So what happens with Alpha now?  Does this cause an error?  I think we discussed this but I forgot the upshot.  Seems like the Alpha prefetches should at least get a warn_once() or something.

src/cpu/simple/timing.cc (Diff revision 1)

yea, again, if prefetches never set fault then this would not be necessary

Summary:

-	ARM: Mark prefetches as such and allow timing CPU to handle them.
+	ARM/Alpha/Cpu: Change prefetchs to be more like normal loads.

Description:

~		ARM: Mark prefetches as such and allow timing CPU to handle them.
	~	ARM/Alpha/Cpu: Change prefetchs to be more like normal loads.
	+
	+	This change modifies the way prefetches work. They are now like normal loads
	+	that don't writeback a register. Previously prefetches were supposed to call
	+	prefetch() on the exection context, so they executed with execute() methods
	+	instead of initiateAcc() completeAcc(). The prefetch() methods for all the CPUs
	+	are blank, meaning that they get executed, but don't actually do anything.
	+
	+	On Alpha dead cache copy code was removed and prefetches are now normal ops.
	+	They count as executed operations, but still don't do anything and IsMemRef is
	+	not longer set on them.
	+
	+	On ARM IsDataPrefetch or IsInstructionPreftech is now set on all prefetch
	+	instructions. The timing simple CPU doesn't try to do anything special for
	+	prefetches now and they execute with the normal memory code path.

Diff:

Revision 2 (+3838 -5588)

Show changes

	src/arch/alpha/isa/decoder.isa
	src/arch/alpha/isa/mem.isa
	src/arch/arm/isa/insts/ldr.isa
	src/cpu/static_inst.hh
	src/cpu/simple/timing.cc
	tests/long/00.gzip/ref/alpha/tru64/o3-timing/config.ini
	tests/long/00.gzip/ref/alpha/tru64/o3-timing/simout
	tests/long/00.gzip/ref/alpha/tru64/o3-timing/stats.txt
	106 more

Please split out the code changes and the stats changes into separate changesets (as is our habit, if not yet our official policy)... the stats changes are just too unwieldy and make it awkward to browse the code changes.  Thanks!

Ali Saidi Nov. 4, 2010, 2:41 a.m. (Nov. 4, 2010, 2:41 a.m.)

I'll post the code changes separately, but when it comes to committing the change I think we should commit code and stats changes together. The goal should be the repository passes regressions at every revision.

src/arch/alpha/isa/decoder.isa (Diff revision 2)

Is there still a writeHint() method out there that just doesn't ever get called now?  Seems like if we're going to change the API to get rid of writeHint() and prefetch() (which seems reasonable to me) we should make sure we eliminate them thoroughly.

Ali Saidi Nov. 4, 2010, 2:40 a.m. (Nov. 4, 2010, 2:40 a.m.)

prefetch() I'm happy with axing. If we wanted to do a write hint type operation in the future, how would we do it? We could do a cache block sized store, in which case I'm happy to axe it too...

src/cpu/simple/timing.cc (Diff revision 2)

Does swapping the order of this OR really matter?

Ali Saidi Nov. 4, 2010, 2:37 a.m. (Nov. 4, 2010, 2:37 a.m.)

Sorry, no... that is what I get for not paying enough attention to my diff. I'll remove that part of it.

Diff:

Revision 3 (+46 -473)

Show changes

	src/arch/alpha/isa/decoder.isa
	src/arch/alpha/isa/mem.isa
	src/arch/arm/isa/insts/ldr.isa
	src/arch/mips/isa/formats/mem.isa
	src/cpu/base_dyn_inst.hh
	src/cpu/base_dyn_inst_impl.hh
	src/cpu/exec_context.hh
	src/cpu/static_inst.hh
	14 more

src/cpu/simple/timing.cc (Diff revision 3)

I still didn't fix this change, but it is fixed in my local patch

Most of these comments are pretty minor, but I think my comment for timing.cc is fairly important. It's not life or death, but there's some low hanging performance there.

src/arch/alpha/isa/mem.isa (Diff revision 3)

You might want to leave out this new blank line.

src/arch/alpha/isa/mem.isa (Diff revision 3)

And getting rid of this blank line.

src/arch/alpha/isa/mem.isa (Diff revision 3)

This doesn't need to be on its own line.

src/cpu/simple/timing.cc (Diff revision 3)

Could this be supressed in initiateAcc itself? That won't work in other models where the fault doesn't go through initiateAcc on its way back to the CPU, but it would make this check unnecessary.

src/cpu/static_inst.hh (Diff revision 3)

You could use isInstPrefetch and isDataPrefetch internally here. It's not likely to ever make a difference, but if those other functions ever get more complicated isPrefetch would pick it up automatically. It's fine if you don't, but I thought I'd suggest it.

You have a pending review.

Review Board 2.0.15

This change has been marked as submitted.

Screenshots

Files

Summary:

Description:

Diff:

Diff: