Clock: Add a Cycles wrapper class and use where applicable

Information
Submitter:	Andreas Hansson
Repository:	gem5
Branch:	default
Bugs:
Depends On:
Reviewers
Groups:	Default
People:

Description

Changeset 9178:eb77cc40e4c6
---------------------------
Clock: Add a Cycles wrapper class and use where applicable

This patch addresses the comments and feedback on the preceding patch
that reworks the clocks and now more clearly shows where cycles
(relative cycle counts) are used to express time.

Instead of bumping the existing patch I chose to make this a separate
patch, merely to try and focus the discussion around a smaller set of
changes. The two patches will be pushed together though.

This changes done as part of this patch are mostly following directly
from the introduction of the wrapper class, and change enough code to
make things compile and run again. There are definitely more places
where int/uint/Tick is still used to represent cycles, and it will
take some time to chase them all down. Similarly, a lot of parameters
should be changed from Param.Tick and Param.Unsigned to
Param.Cycles.

In addition, the use of curTick is questionable as there should not be
an absolute cycle. Potential solutions can be built on top of this
patch. There is a similar situation in the o3 CPU where
lastRunningCycle is currently counting in Cycles, and is still an
absolute time. More discussion to be had in other words.

An additional change that would be appropriate in the future is to
perform a similar wrapping of Tick and probably also introduce a
Ticks class along with suitable operators for all these classes.

Testing Done

util/regress all passing (disregarding t1000 and eio)

Issue Summary

Description	From	Last Updated	Status
Seems that this should actually be params.Frequency	Nathan Binkert	Aug. 22, 2012, 10:17 p.m.	Resolved
The CPUProgrress event is really in terms of ticks right now, so I don't think that you want this. Frequency ...	Nathan Binkert	Aug. 22, 2012, 10:17 p.m.	Resolved
You can always wrap code with #ifndef SWIG to avoid having swig parse stuff. Seems like you'd also want full ...	Nathan Binkert	Aug. 22, 2012, 10:19 p.m.	Resolved
The family of operator= should really return a const reference.	Nathan Binkert	Aug. 22, 2012, 10:19 p.m.	Resolved
Any reason this shouldn't be private?	Nathan Binkert	Aug. 24, 2012, 3:15 a.m.	Resolved
How about equality (or all of them really)? Is that really never used?	Nathan Binkert	Aug. 24, 2012, 7:52 a.m.	Dropped

one use of uint... don't need to rereview.

I'm honestly not a big fan of CycleCount.  I'd much rather see Cycle.  Is there something already called Cycle that would clash?  I know it's not as descriptive, but it's used all over the place and shouldn't take long to get used to.  Tick isn't TickCount.

Ali Saidi Aug. 7, 2012, 5:12 a.m. (Aug. 7, 2012, 5:12 a.m.)

The reason Tick isn't TickCount is Tick is absolute, while CycleCount is supposed to only be relative.

Nathan Binkert Aug. 7, 2012, 5:13 a.m. (Aug. 7, 2012, 5:13 a.m.)
```
How about "Cycles" then :)
```

Andreas Hansson Aug. 7, 2012, 6:14 p.m. (Aug. 7, 2012, 6:14 p.m.)

I'm all in favour of something shorter as well, but I do see the point of making it more explicit that it is relative.

That said, there are plenty places where the type Tick is used to denote a relative value (the diff between curTick and some other absolute time for example).

I am happy with either really, and merely followed the conclusion from our e-mail thread. What does the rest of you guys think? Stay with CycleCount to make it explicit?

src/cpu/BaseCPU.py (Diff revision 1)

Seems that this should actually be params.Frequency

Show all issues

src/cpu/BaseCPU.py (Diff revision 1)

The CPUProgrress event is really in terms of ticks right now, so I don't think that you want this.  Frequency would probably be most appropriate.  If you want it to be a multiple of clock cycles, then I think you need to do more work.  (Though with that other diff I sent, multiple of cycles works with "1c")

Show all issues

src/sim/clocked_object.hh (Diff revision 1)

You can always wrap code with #ifndef SWIG to avoid having swig parse stuff.

Seems like you'd also want full comparisons.

Show all issues

src/sim/clocked_object.hh (Diff revision 1)

The family of operator= should really return a const reference.

Show all issues

Description:

~		Changeset 9144:2056fa4fe069
	~	Changeset 9152:6ecd7b9ddc93

		Clock: Add a CycleCount wrapper class and use where applicable

		This patch addresses the comments and feedback on the preceding patch
		that reworks the clocks and now more clearly shows where cycles (or
		rather cycle counts) are used to express time.

		Instead of bumping the existing patch I chose to make this a separate
		patch, merely to try and focus the discussion around a smaller set of
		changes. The two patches will be pushed together though.

		This changes done as part of this patch are mostly following directly
		from the introduction of the wrapper class, and change enough code to
		make things compile and run again. There are definitely more places
		where int/uint/Tick is still used to represent cycles, and it will
		take some time to chase them all down. Similarly, a lot of parameters
		should be changed from Param.Tick and Param.Unsigned to
		Param.CycleCount.

		An additional change that would be appropriate in the future is to
		perform a similar wrapping of Tick and probably also introduce a
		TickCount along with suitable operators for all these classes.

Diff:

Revision 2 (+323 -242)

Show changes

	src/arch/alpha/mmapped_ipr.hh
	src/arch/alpha/utility.hh
	src/arch/arm/mmapped_ipr.hh
	src/arch/arm/table_walker.cc
	src/arch/arm/utility.hh
	src/arch/mips/isa.hh
	src/arch/mips/isa.cc
	src/arch/mips/mmapped_ipr.hh
	73 more

Description:

~		Changeset 9152:6ecd7b9ddc93
	~	Changeset 9169:596c3559adda

		Clock: Add a CycleCount wrapper class and use where applicable

		This patch addresses the comments and feedback on the preceding patch
		that reworks the clocks and now more clearly shows where cycles (or
		rather cycle counts) are used to express time.

		Instead of bumping the existing patch I chose to make this a separate
		patch, merely to try and focus the discussion around a smaller set of
		changes. The two patches will be pushed together though.

		This changes done as part of this patch are mostly following directly
		from the introduction of the wrapper class, and change enough code to
		make things compile and run again. There are definitely more places
		where int/uint/Tick is still used to represent cycles, and it will
		take some time to chase them all down. Similarly, a lot of parameters
		should be changed from Param.Tick and Param.Unsigned to
		Param.CycleCount.

		An additional change that would be appropriate in the future is to
		perform a similar wrapping of Tick and probably also introduce a
		TickCount along with suitable operators for all these classes.

Diff:

Revision 3 (+339 -257)

Show changes

	src/arch/alpha/mmapped_ipr.hh
	src/arch/alpha/utility.hh
	src/arch/arm/mmapped_ipr.hh
	src/arch/arm/table_walker.cc
	src/arch/arm/utility.hh
	src/arch/mips/isa.hh
	src/arch/mips/isa.cc
	src/arch/mips/mmapped_ipr.hh
	75 more

Summary:

-	Clock: Add a CycleCount wrapper class and use where applicable
+	Clock: Add a Cycles wrapper class and use where applicable

Description:

~		Changeset 9169:596c3559adda
	~	Changeset 9169:9c3c49e0eb85

~		Clock: Add a CycleCount wrapper class and use where applicable
	~	Clock: Add a Cycles wrapper class and use where applicable

		This patch addresses the comments and feedback on the preceding patch
~		that reworks the clocks and now more clearly shows where cycles (or
~		rather cycle counts) are used to express time.
	~	that reworks the clocks and now more clearly shows where cycles
	~	(relative cycle counts) are used to express time.

		Instead of bumping the existing patch I chose to make this a separate
		patch, merely to try and focus the discussion around a smaller set of
		changes. The two patches will be pushed together though.

		This changes done as part of this patch are mostly following directly
		from the introduction of the wrapper class, and change enough code to
		make things compile and run again. There are definitely more places
		where int/uint/Tick is still used to represent cycles, and it will
		take some time to chase them all down. Similarly, a lot of parameters
		should be changed from Param.Tick and Param.Unsigned to
~		Param.CycleCount.
	~	Param.Cycles.
	+
	+	In addition, the use of curTick is questionable as there should not be
	+	an absolute cycle. Potential solutions can be built on top of this
	+	patch. There is a similar situation in the o3 CPU where
	+	lastRunningCycle is currently counting in Cycles, and is still an
	+	absolute time. More discussion to be had in other words.

		An additional change that would be appropriate in the future is to
		perform a similar wrapping of Tick and probably also introduce a
~		TickCount along with suitable operators for all these classes.
	~	Ticks class along with suitable operators for all these classes.

Diff:

Revision 4 (+339 -257)

Show changes

	src/arch/alpha/mmapped_ipr.hh
	src/arch/alpha/utility.hh
	src/arch/arm/mmapped_ipr.hh
	src/arch/arm/table_walker.cc
	src/arch/arm/utility.hh
	src/arch/mips/isa.hh
	src/arch/mips/isa.cc
	src/arch/mips/mmapped_ipr.hh
	75 more

I looked at the big code blocks in depth and skimmed the rest.  The biggest bizarre thing to me is where there is a comment about thread ID and you replace the value with Cycles.  I looked at the declarations and it looks like your right, but is there any breakage from the threadid thing or are they all just bad comments?

Andreas Hansson Aug. 24, 2012, 3:16 a.m. (Aug. 24, 2012, 3:16 a.m.)

Where is this exactly? I'll have another look. All the regressions are unchanged with the current patch.

src/base/types.hh (Diff revision 4)

Any reason this shouldn't be private?

Show all issues

Andreas Hansson Aug. 24, 2012, 3:16 a.m. (Aug. 24, 2012, 3:16 a.m.)
```
Fixed and re-running.
```

src/base/types.hh (Diff revision 4)

How about equality (or all of them really)?  Is that really never used?

Show all issues

Andreas Hansson Aug. 24, 2012, 3:16 a.m. (Aug. 24, 2012, 3:16 a.m.)

I only added the ones that are currently used. I would suggest to add or remove as needed rather than doing it pro-actively. Agreed?

Nathan Binkert Aug. 24, 2012, 6:54 a.m. (Aug. 24, 2012, 6:54 a.m.)

We just have two different, valid, approaches to this.  No sweat.

Description:

~		Changeset 9169:9c3c49e0eb85
	~	Changeset 9178:eb77cc40e4c6

		Clock: Add a Cycles wrapper class and use where applicable

		This patch addresses the comments and feedback on the preceding patch
		that reworks the clocks and now more clearly shows where cycles
		(relative cycle counts) are used to express time.

		Instead of bumping the existing patch I chose to make this a separate
		patch, merely to try and focus the discussion around a smaller set of
		changes. The two patches will be pushed together though.

		This changes done as part of this patch are mostly following directly
		from the introduction of the wrapper class, and change enough code to
		make things compile and run again. There are definitely more places
		where int/uint/Tick is still used to represent cycles, and it will
		take some time to chase them all down. Similarly, a lot of parameters
		should be changed from Param.Tick and Param.Unsigned to
		Param.Cycles.

		In addition, the use of curTick is questionable as there should not be
		an absolute cycle. Potential solutions can be built on top of this
		patch. There is a similar situation in the o3 CPU where
		lastRunningCycle is currently counting in Cycles, and is still an
		absolute time. More discussion to be had in other words.

		An additional change that would be appropriate in the future is to
		perform a similar wrapping of Tick and probably also introduce a
		Ticks class along with suitable operators for all these classes.

Diff:

Revision 5 (+340 -256)

Show changes

	src/arch/alpha/mmapped_ipr.hh
	src/arch/alpha/utility.hh
	src/arch/arm/mmapped_ipr.hh
	src/arch/arm/table_walker.cc
	src/arch/arm/utility.hh
	src/arch/mips/isa.hh
	src/arch/mips/isa.cc
	src/arch/mips/mmapped_ipr.hh
	75 more

You have a pending review.

Review Board 2.0.15

This change has been marked as submitted.

Screenshots

Files

Issue Summary

Description:

Changeset 9144:2056fa4fe069

Changeset 9152:6ecd7b9ddc93

Diff:

Description:

Changeset 9152:6ecd7b9ddc93

Changeset 9169:596c3559adda

Diff:

Summary:

Description:

Changeset 9169:596c3559adda

Changeset 9169:9c3c49e0eb85

Diff:

Description:

Changeset 9169:9c3c49e0eb85

Changeset 9178:eb77cc40e4c6

Diff:

Status: Closed (submitted)