CPU: Merge the predecoder and decoder.

Information
Submitter:	Gabe Black
Repository:	gem5
Branch:	default
Bugs:
Depends On:
Reviewers
Groups:	Default
People:

Description

Changeset 9023:ea2094694097
---------------------------
CPU: Merge the predecoder and decoder.

These classes are always used together, and merging them will give the ISAs
more flexibility in how they cache things and manage the process.

Testing Done

Issue Summary

Description	From	Last Updated	Status
This function name "extMachInstReady()" seems wrong, since the ExtMachInst is not visible outside the Decoder (right?)	Steve Reinhardt	May 20, 2012, 8:20 a.m.	Open
Why aren't we skipping the ExtMachInst completely here, and just doing instPtr = thread->decoder.decode(pcState) ?	Steve Reinhardt	May 20, 2012, 10:44 a.m.	Open
same comment as above... this function name makes no sense in the modified context where the code doesn't mention extMachInst ...	Steve Reinhardt	May 20, 2012, 10:44 a.m.	Open
the concept of "extMachInst" is meaningless now, so this function name doesn't make sense in context. should it be decoderReady() ...	Steve Reinhardt	May 20, 2012, 10:44 a.m.	Open
similar thing here... how is !extMachInstReady() at this point different from needMoreBytes()? The latter sounds more intuitively like what you ...	Steve Reinhardt	May 20, 2012, 10:44 a.m.	Open
My only concern with the Checker code is: does getName() have a chance for aliasing between instructions with slightly different ...	Geoffrey Blake	May 26, 2012, 7:17 a.m.	Open

In general I'm in agreement with this direction (as we've already discussed, but I just wanted to be clear about it).

I'm curious why functions like extMachInstReady() and getExtMachInst() are still public... ideally I'd think that the ExtMachInst type(s) would now be entirely internal to the Decoder objects, and their definitions (not to mention their very existence) would be invisible to the CPU models.

Gabe Black May 20, 2012, 8:38 a.m. (May 20, 2012, 8:38 a.m.)

That's a very reasonable thing to move towards, but outside of the scope of what I'm trying to do. I started looking at it, and there are two problems (which are not show stoppers) which make this less straightforward than it could be. First, there's the noop ExtMachInst which is used, notably in O3, to transport faults through the pipe. I think we definitely should have a generic, ISA independent noop which can just be used without going through the decoder, but because of the funny way the execute methods work it's not possible, as far as I can see, to define an instruction object that can actually be used outside of the ISA parser. I would like to change that generally speaking by coming up with a C++ based mechanism which works outside of generated code, but in the mean time that's fairly intractable. Second, the idea of an ExtMachInst is plumbed all the way down into the base StaticInst class. Extracting it and putting ExtMachInsts into the ISA specific classes that actually need it shouldn't be very complicated at all, but it's still a decent amount of work and outside the scope of what I'm trying to do here.

Steve Reinhardt May 20, 2012, 10:45 a.m. (May 20, 2012, 10:45 a.m.)

Fair enough, I can believe it's not that simple to get rid of all traces of ExtMachInst outside the Decoder, even if that's the eventual goal.  It still seems like extMachInstReady() should be renamed and getExtMachInst() made private though.  Do we really care if the ExtMachInst is ready but not the StaticInst?  I pointed out some specific examples in a separate review I just published.

src/cpu/checker/cpu_impl.hh (Diff revision 1)

This function name "extMachInstReady()" seems wrong, since the ExtMachInst is not visible outside the Decoder (right?)

Show all issues

Gabe Black May 20, 2012, 8:38 a.m. (May 20, 2012, 8:38 a.m.)
```
Nope, at least not yet anyway.
```

Gabe Black May 20, 2012, 10:54 a.m. (May 20, 2012, 10:54 a.m.)

I meant to also say that I'm not trying to change the interface to the decoder/predecoder combo here. That would come later and is outside the scope of this change.

Steve Reinhardt May 20, 2012, 11:15 a.m. (May 20, 2012, 11:15 a.m.)

OK, I guess I'm just confused because there are places where you did change the interface more significantly, like the change in cpu/simple/base.cc around lines 380-400, getting rid of extMachInstReady() and getExtMachInst() entirely, and I was hoping to see that kind of transformation everywhere that those functions were used.

src/cpu/checker/cpu_impl.hh (Diff revision 1)

Why aren't we skipping the ExtMachInst completely here, and just doing
instPtr = thread->decoder.decode(pcState)
?

Show all issues

Gabe Black May 20, 2012, 10:52 a.m. (May 20, 2012, 10:52 a.m.)

Because I don't want to change how the checker works while I'm working on something else.

src/cpu/inorder/resources/fetch_unit.cc (Diff revision 1)

same comment as above... this function name makes no sense in the modified context where the code doesn't mention extMachInst otherwise

Show all issues

src/cpu/legiontrace.cc (Diff revision 1)

the concept of "extMachInst" is meaningless now, so this function name doesn't make sense in context.  should it be decoderReady() or something like that?

Show all issues

Gabe Black May 20, 2012, 10:52 a.m. (May 20, 2012, 10:52 a.m.)

It isn't meaningless yet. Also, the decoder being ready is ambiguous. Ready for what? More bytes? To return an instruction?

Steve Reinhardt May 20, 2012, 11:15 a.m. (May 20, 2012, 11:15 a.m.)

Well what does extMachInstReady() mean when we're not using it to decide whether to call getExtMachInst() anymore?

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

similar thing here... how is !extMachInstReady() at this point different from needMoreBytes()?  The latter sounds more intuitively like what you want to call based on the name of the flag.

Show all issues

Gabe Black May 20, 2012, 10:52 a.m. (May 20, 2012, 10:52 a.m.)

!extMachInstReady is not the same as needMoreBytes at all. There may be many instructions in the same MachInst blob of bytes. There may be many MachInst blobs before we get a complete instruction.

Steve Reinhardt May 20, 2012, 11:15 a.m. (May 20, 2012, 11:15 a.m.)

I figured that they weren't the same, but I don't understand the difference.  Intuitively I'd think that needMoreBytes() means "need more bytes to make a complete instruction".  I don't see any comments that explain the difference.

Anyway, I'm not sure where this interface is headed, but given some of the changes you made, it seems like some additional changes like renaming extMachInstReady() to have a name that's meaningful even when you're not calling getExtMachInst() (assuming it's not about to go away entirely), and making uniform changes to places where instructions get decoded (i.e., taking whatever you did in simple/base.cc and doing it to the checker too), would help to make this change overall more consistent.

Description:

~		Changeset 9009:769d2142cbd4
	~	Changeset 9023:ea2094694097

		CPU: Merge the predecoder and decoder.

		These classes are always used together, and merging them will give the ISAs
		more flexibility in how they cache things and manage the process.

Diff:

Revision 2 (+1320 -1747)

Show changes

	src/arch/SConscript
	src/arch/alpha/decoder.hh
	src/arch/alpha/predecoder.hh
	src/arch/alpha/isa/main.isa
	src/arch/arm/SConscript
	src/arch/arm/decoder.hh
	src/arch/arm/decoder.cc
	src/arch/arm/predecoder.hh
	36 more

Thanks for the changes!

Status: Closed (submitted)

src/cpu/checker/cpu_impl.hh (Diff revision 2)

My only concern with the Checker code is: does getName() have a chance for aliasing between instructions with slightly different binary representations but have the same mnemonic?  Otherwise removing the dependency on machInst allows the Checker to be more easily adapted to checking other ISAs besides ARM.

Show all issues

Gabe Black May 26, 2012, 7:27 a.m. (May 26, 2012, 7:27 a.m.)

Yes it does a chance for aliasing, but getName isn't being used for comparison, only for the warning text. This should actually make it more easily adapted to other ISAs because it doesn't make assumptions about whether an ExtMachInst can be cast back to a MachInst.

You have a pending review.

Review Board 2.0.15

This change has been marked as submitted.

Screenshots

Files

Issue Summary

Description:

Changeset 9009:769d2142cbd4

Changeset 9023:ea2094694097

Diff:

Status: Closed (submitted)