cpu/o3: clean up rename map and free list

Information
Submitter:	Steve Reinhardt
Repository:	gem5
Branch:	default
Bugs:
Depends On:
Reviewers
Groups:	Default
People:

Description

Changeset 9852:d1f17f13ef26
---------------------------
cpu/o3: clean up rename map and free list

Restructured rename map and free list to clean up some
extraneous code and separate out common code that can
be reused across different register classes (int and fp
at this point).  Both components now consist of a set
of Simple* objects that are stand-alone rename map &
free list for each class, plus a Unified* object that
presents a unified interface across all register
classes and then redirects accesses to the appropriate
Simple* object as needed.

Moved free list initialization to PhysRegFile to better
isolate knowledge of physical register index mappings
to that class (and remove the need to pass a number
of parameters to the free list constructor).

Causes a small change to these stats:
  cpu.rename.int_rename_lookups
  cpu.rename.fp_rename_lookups
because they are now categorized on a per-operand basis
rather than a per-instruction basis.
That is, an instruction with mixed fp/int/misc operand
types will have each operand categorized independently,
where previously the lookup was categorized based on
the instruction type.

Testing Done

Issue Summary

Description	From	Last Updated	Status
queue is certainly nice for readability here, but i'm wondering (and not saynig to go do this), but if a ...	Ali Saidi	Aug. 27, 2013, 5:03 p.m.	Resolved
const	Ali Saidi	Aug. 26, 2013, 7:18 p.m.	Resolved
cost	Ali Saidi	Aug. 26, 2013, 7:18 p.m.	Resolved
Spacing around -, also extraneous parentheses around RegIndex	Andreas Hansson	Aug. 27, 2013, 5:03 p.m.	Resolved
bool?	Andreas Hansson	Aug. 26, 2013, 7:22 p.m.	Resolved
Somehow the parameters do not seem matched with the signature here.	Andreas Hansson	Aug. 26, 2013, 7:54 p.m.	Dropped
const	Andreas Hansson	Aug. 26, 2013, 8:16 p.m.	Dropped
const	Andreas Hansson	Aug. 26, 2013, 8:16 p.m.	Dropped
const	Andreas Hansson	Aug. 26, 2013, 7:21 p.m.	Resolved
const	Andreas Hansson	Aug. 26, 2013, 7:21 p.m.	Resolved
const	Andreas Hansson	Aug. 26, 2013, 7:21 p.m.	Resolved
const	Andreas Hansson	Aug. 26, 2013, 7:21 p.m.	Resolved
Should not the cpu bit be the parent name, and should it not also contain the system bit etc?	Andreas Hansson	Aug. 26, 2013, 8:14 p.m.	Resolved
I assume there is a good reason to not make these std::vectors.	Andreas Hansson	Aug. 26, 2013, 8:12 p.m.	Resolved
freeList(NULL)?	Andreas Hansson	Aug. 26, 2013, 7:50 p.m.	Resolved
const	Andreas Hansson	Aug. 26, 2013, 7:23 p.m.	Resolved
const?	Andreas Hansson	Aug. 26, 2013, 7:24 p.m.	Resolved
const	Andreas Hansson	Aug. 26, 2013, 7:24 p.m.	Resolved
const	Andreas Hansson	Aug. 26, 2013, 7:24 p.m.	Resolved
const	Andreas Hansson	Aug. 26, 2013, 7:24 p.m.	Resolved
const unsigned here and for numFreeEntries in the maps?	Andreas Hansson	Aug. 26, 2013, 7:48 p.m.	Resolved
Assert(freeList == NULL && map.empty());	Andreas Hansson	Aug. 26, 2013, 7:50 p.m.	Resolved

Ali Saidi Aug. 24, 2013, 2:02 p.m. (Aug. 24, 2013, 2:02 p.m.)
```
otherwise ship it.
```

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

queue is certainly nice for readability here, but i'm wondering (and not saynig to go do this), but if a vector would actually be faster if you treated it like a stack. It's just a bit funny that we typedef the size so we can have a 16 byte index and two 64 byte pointers.

Show all issues

Steve Reinhardt Aug. 27, 2013, 5:08 p.m. (Aug. 27, 2013, 5:08 p.m.)

I switched it to std::stack<PhysRegIndex> and that seems to work.  The docs I found says that defaults to using a deque as the underlying container.  It's a one-line change to go to std::stack<PhysRegIndex, std::vector<PhysRegIndex> > if you think that's preferable.

Steve Reinhardt Aug. 27, 2013, 8:55 p.m. (Aug. 27, 2013, 8:55 p.m.)

Spoke too soon... it turns out that the O3 model relies on the physical zero register having the same index as the logical zero register <shudder>.  This happens to be the case when you push all the physical registers (starting at index zero) on a queue, then pull them off the head when you do the initial assignment to logical registers (also starting at index zero).  When you replace the queue with a stack, you break this happy coincidence, and it stops working.  So I'll leave it as a queue for now... with some effort, we could convert it to a stack, but I think that's best left for a separate patch.

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

const

Show all issues

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

cost

Show all issues

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

Spacing around -, also extraneous parentheses around RegIndex

Show all issues

Steve Reinhardt Aug. 27, 2013, 5:09 p.m. (Aug. 27, 2013, 5:09 p.m.)

This is actually a cast of a negative 1, not a subtraction... I revamped it to be (I hope) less confusing.

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

bool?

Show all issues

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

Somehow the parameters do not seem matched with the signature here.

Show all issues

Steve Reinhardt Aug. 27, 2013, 5:09 p.m. (Aug. 27, 2013, 5:09 p.m.)

I'm not sure what you mean... looks OK to me.  I think the diff is misleading, as I took what was one class and split it into two.

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

const

Show all issues

Steve Reinhardt Aug. 27, 2013, 5:09 p.m. (Aug. 27, 2013, 5:09 p.m.)

Actually this is not const, it's allocating a free reg from the list (and removing it).

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

const

Show all issues

Steve Reinhardt Aug. 27, 2013, 5:09 p.m. (Aug. 27, 2013, 5:09 p.m.)
```
same here
```

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

const

Show all issues

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

const

Show all issues

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

const

Show all issues

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

const

Show all issues

src/cpu/o3/free_list.cc (Diff revision 1)

Should not the cpu bit be the parent name, and should it not also contain the system bit etc?

Show all issues

src/cpu/o3/regfile.cc (Diff revision 1)

I assume there is a good reason to not make these std::vectors.

Show all issues

Steve Reinhardt Aug. 27, 2013, 5:09 p.m. (Aug. 27, 2013, 5:09 p.m.)

No good reason... making them vectors does clean up initialization & destruction a bit.

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

freeList(NULL)?

Show all issues

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

const

Show all issues

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

const?

Show all issues

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

const

Show all issues

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

const

Show all issues

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

const

Show all issues

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

const

unsigned here and for numFreeEntries in the maps?

Show all issues

src/cpu/o3/rename_map.cc (Diff revision 1)

Assert(freeList == NULL && map.empty());

Show all issues

Some minor bits and pieces

Description:

~		Changeset 9850:65d5069b045e
	~	Changeset 9852:b30933fc9697

		cpu/o3: clean up rename map and free list

		Restructured rename map and free list to clean up some
		extraneous code and separate out common code that can
		be reused across different register classes (int and fp
		at this point). Both components now consist of a set
		of Simple objects that are stand-alone rename map &
		free list for each class, plus a Unified object that
		presents a unified interface across all register
		classes and then redirects accesses to the appropriate
		Simple* object as needed.

		Moved free list initialization to PhysRegFile to better
		isolate knowledge of physical register index mappings
		to that class (and remove the need to pass a number
		of parameters to the free list constructor).

		Causes a small change to these stats:
		cpu.rename.int_rename_lookups
		cpu.rename.fp_rename_lookups
		because they are now categorized on a per-operand basis
		rather than a per-instruction basis.
		That is, an instruction with mixed fp/int/misc operand
		types will have each operand categorized independently,
		where previously the lookup was categorized based on
		the instruction type.

Diff:

Revision 2 (+592 -532)

Show changes

	src/cpu/o3/SConscript
	src/cpu/o3/cpu.hh
	src/cpu/o3/cpu.cc
	src/cpu/o3/cpu_policy.hh
	src/cpu/o3/free_list.hh
	src/cpu/o3/free_list.cc
	src/cpu/o3/regfile.hh
	src/cpu/o3/regfile.cc
	3 more

Description:

~		Changeset 9852:b30933fc9697
	~	Changeset 9852:d1f17f13ef26

		cpu/o3: clean up rename map and free list

		Restructured rename map and free list to clean up some
		extraneous code and separate out common code that can
		be reused across different register classes (int and fp
		at this point). Both components now consist of a set
		of Simple objects that are stand-alone rename map &
		free list for each class, plus a Unified object that
		presents a unified interface across all register
		classes and then redirects accesses to the appropriate
		Simple* object as needed.

		Moved free list initialization to PhysRegFile to better
		isolate knowledge of physical register index mappings
		to that class (and remove the need to pass a number
		of parameters to the free list constructor).

		Causes a small change to these stats:
		cpu.rename.int_rename_lookups
		cpu.rename.fp_rename_lookups
		because they are now categorized on a per-operand basis
		rather than a per-instruction basis.
		That is, an instruction with mixed fp/int/misc operand
		types will have each operand categorized independently,
		where previously the lookup was categorized based on
		the instruction type.

Diff:

Revision 3 (+597 -538)

Show changes

	src/cpu/o3/SConscript
	src/cpu/o3/cpu.hh
	src/cpu/o3/cpu.cc
	src/cpu/o3/cpu_policy.hh
	src/cpu/o3/free_list.hh
	src/cpu/o3/free_list.cc
	src/cpu/o3/regfile.hh
	src/cpu/o3/regfile.cc
	3 more

Ship It!

You have a pending review.

Review Board 2.0.15

This change has been marked as submitted.

Screenshots

Files

Issue Summary

Description:

Changeset 9850:65d5069b045e

Changeset 9852:b30933fc9697

Diff:

Description:

Changeset 9852:b30933fc9697

Changeset 9852:d1f17f13ef26

Diff:

Status: Closed (submitted)