Configs: Fix up maxtick and maxtime

Information
Submitter:	Joel Hestness
Repository:	gem5
Branch:	default
Bugs:
Depends On:
Reviewers
Groups:	Default
People:

Description

Changeset 9812:ddc50e6eae13
---------------------------
Configs: Fix up maxtick and maxtime

This patch contains three fixes to max tick options handling in Options.py and
Simulation.py:

 1) Since the global simulator frequency isn't bound until m5.instantiate()
is called, the maxtick resolution needs to happen after this call, since
changes to the global frequency will cause m5.simulate() to misinterpret the
maxtick value. Shuffling this also requires tweaking the checkpoint directory
handling to signal the checkpoint restore tick back to run().  Fixing this
completely and correctly will require storing the simulation frequency into
checkpoints, which is beyond the scope of this patch.

 2) The maxtick option in Options.py was defaulted to MaxTicks, so the old code
would always skip over the maxtime part of the conditionals at the beginning
of run(). Change the maxtick default to None, and set the maxtick local
variable in run() appropriately.

 3) To clarify whether max ticks settings are relative or absolute, split the
maxtick option into separate options, for relative and absolute. Ensure that
these two options and the maxtime option are handled appropriately to set the
maxtick variable in Simulation.py.

Testing Done

Ship It!

Description:

~		Changeset 9633:68b31b29f5ad
	~	Changeset 9633:dfd9f65d2011

		Simulate.py: Fix up maxtick and maxtime

		This patch contains a couple fixes:

		1) Since the global simulator frequency isn't bound until m5.instantiate()
		is called, the maxtick resolution needs to happen after this call, since
		changes to the global frequency will cause m5.simulate() to misinterpret the
		maxtick value. Shuffling this also requires tweaking the checkpoint directory
		handling to signal the checkpoint restore tick back to run(). Fixing this
		completely and correctly will require storing the simulation frequency into
		checkpoints, which is beyond the scope of this patch.

		2) The maxtick option in Options.py was defaulted to MaxTicks, so the old code
		would always skip over the maxtime part of the conditionals at the beginning
		of run(). Change the maxtick default to None, and set the maxtick local
		variable in run() appropriately.

Diff:

Revision 2 (+22 -18)

Show changes

	configs/common/Options.py
	configs/common/Simulation.py

From the other patch review:

Jason Power:
I just now was looking at your other patch and I see that for the option maxtick the help says help="Stop after T ticks". This is ambiguous as to what the expected behavior is. Can you update the help message in this patch to clarify exactly what the behavior should be.

Andreas Hansson:
Agreed, the "Run to" and "Stop after" are both rather ambiguous. It would be good if it was made more clear what is absolute (if anything) and what is relative.

Joel Hestness April 9, 2013, 2:06 a.m. (April 9, 2013, 2:06 a.m.)

Ok. This raises a couple questions:
1) Do we want the maxtick and maxtime options to be relative to a restored checkpoint, or include the time from the restore?
 
I can never remember which I need to use when I use maxticks when restoring from a checkpoint. That said, I usually guess (incorrectly) that the maxtick is relative to the restore time, which would be consistent with the way that one thinks about maxtick when not restoring from checkpoint. If no one minds, I'd prefer to change the handling to be relative to the restore time for both of these options.

2) Do we care if the help message grows to a couple sentences?

If we decide to leave these variables as absolute time, there isn't really a good way to explain that in a single sentence, because it requires explaining that it includes time from the checkpoint that will be restored.

However we decide, I'll update the patch and resubmit.

Ali Saidi April 10, 2013, 1:24 p.m. (April 10, 2013, 1:24 p.m.)

I can see use cases for both absolute time and relative time, I almost wonder if we should have a --abs-max-tick and --rel-max-tick to solve the problem. 

I'm all for the help message growing.

Andreas Hansson July 16, 2013, 4:01 p.m. (July 16, 2013, 4:01 p.m.)
```
What happened with this one?
```

Joel Hestness July 17, 2013, 11:05 a.m. (July 17, 2013, 11:05 a.m.)

I haven't had a chance to merge this up to the latest repo and add relative vs. absolute max ticks. I'll try to tackle this today.

Summary:

-	Simulate.py: Fix up maxtick and maxtime
+	Configs: Fix up maxtick and maxtime

Description:

~		Changeset 9633:dfd9f65d2011
	~	Changeset 9812:ddc50e6eae13

~		Simulate.py: Fix up maxtick and maxtime
	~	Configs: Fix up maxtick and maxtime

~		This patch contains a couple fixes:
	~	This patch contains three fixes to max tick options handling in Options.py and
	+	Simulation.py:

		1) Since the global simulator frequency isn't bound until m5.instantiate()
		is called, the maxtick resolution needs to happen after this call, since
		changes to the global frequency will cause m5.simulate() to misinterpret the
		maxtick value. Shuffling this also requires tweaking the checkpoint directory
		handling to signal the checkpoint restore tick back to run(). Fixing this
		completely and correctly will require storing the simulation frequency into
		checkpoints, which is beyond the scope of this patch.

		2) The maxtick option in Options.py was defaulted to MaxTicks, so the old code
		would always skip over the maxtime part of the conditionals at the beginning
		of run(). Change the maxtick default to None, and set the maxtick local
		variable in run() appropriately.
	+
	+	3) To clarify whether max ticks settings are relative or absolute, split the
	+	maxtick option into separate options, for relative and absolute. Ensure that
	+	these two options and the maxtime option are handled appropriately to set the
	+	maxtick variable in Simulation.py.

Diff:

Revision 3 (+45 -20)

Show changes

	configs/common/Options.py
	configs/common/Simulation.py

Thanks! Looks great

You have a pending review.

Review Board 2.0.15

This change has been marked as submitted.

Screenshots

Files

Description:

Changeset 9633:68b31b29f5ad

Changeset 9633:dfd9f65d2011

Diff:

Summary:

Description:

Changeset 9633:dfd9f65d2011

Changeset 9812:ddc50e6eae13

Diff:

Status: Closed (submitted)