config: Add a BaseSESystem builder for re-use in regressions

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

Description

Changeset 9773:57a236cff5e9
---------------------------
config: Add a BaseSESystem builder for re-use in regressions

This patch extends the existing system builders to also include a
syscall-emulation builder. This builder is deployed in all
syscall-emulation regressions that do not involve Ruby,
i.e. o3-timing, simple-timing and simple-atomic, as well as the
multi-processor regressions o3-timing-mp, simple-timing-mp and
simple-atomic-mp (the latter are only used by SPARC at this point).

The values chosen for the cache sizes match those that were used in
the existing config scripts (despite being on the large
side). Similarly, a mem_class parameter is added to the builder base
class to enable simple-atomic to use SimpleMemory and o3-timing to use
the default DDR3 configuration.

Due to the different order the ports are connected, the bus stats get
shuffled around for the multi-processor regressions. A separate patch
bumps the port indices. Besides this, all behaviour is exactly the
same.

Testing Done

All regressions pass

Issue Summary

Description	From	Last Updated	Status
Adding the mem_class parameter should really be in a separate commit or at least be mentioned in the commit message.	Andreas Sandberg	June 10, 2013, 8:18 p.m.	Resolved

Seems fine to me.

LGTM except for the comment below.

tests/configs/base_config.py (Diff revision 1)

Adding the mem_class parameter should really be in a separate commit or at least be mentioned in the commit message.

Show all issues

Description:

~		Changeset 9757:47d39456a0ec
	~	Changeset 9772:b3272cd98ff7

		config: Add a BaseSESystem builder for re-use in regressions

		This patch extends the existing system builders to also include a
		syscall-emulation base builder. To start with, this builder is only
		deployed in a limited number of cases, most notably o3-timing,
		simple-timing and simple-atomic. The main reason for this is to not
		change any regressions.

		At this point only a uni-processor builder is added, and the values
		chosen for the cache sizes match those that were used in the existing
		config scripts (despite being on the large side).
	+
	+	A mem_class parameter is added to the builder base class to enable
	+	simple-atomic to use SimpleMemory and o3-timing to use the default
	+	DDR3 configuration.

Diff:

Revision 2 (+139 -152)

Show changes

	tests/configs/base_config.py
	tests/configs/inorder-timing.py
	tests/configs/o3-timing.py
	tests/configs/o3-timing-checker.py
	tests/configs/simple-atomic.py
	tests/configs/simple-atomic-dummychecker.py
	tests/configs/simple-timing.py
	tests/long/se/10.mcf/test.py
	10 more

Looks great, I'm very glad to see all this code factored out.  Just a couple of minor things:

- When you change a file, please just add the ARM copyright without replacing the existing one, even if 99% of the file has been replaced.  Nothing personal, but given how ARM gets added to the copyright list even for the most minor changes, it seems asymmetric to remove others' copyrights when the file has not been 100% replaced with something completely different.

- I don't understand the logic of splitting this patch from the following one (1902), since you're already modifying some of the files under test/configs here.  I think it's less confusing to have all the related changes in a single patch, but even if I were going to split it in two, I don't think I would choose this split.

Description:

~		Changeset 9772:b3272cd98ff7
	~	Changeset 9772:b914860111a9

		config: Add a BaseSESystem builder for re-use in regressions

		This patch extends the existing system builders to also include a
		syscall-emulation base builder. To start with, this builder is only
		deployed in a limited number of cases, most notably o3-timing,
		simple-timing and simple-atomic. The main reason for this is to not
		change any regressions.

		At this point only a uni-processor builder is added, and the values
		chosen for the cache sizes match those that were used in the existing
		config scripts (despite being on the large side).

		A mem_class parameter is added to the builder base class to enable
		simple-atomic to use SimpleMemory and o3-timing to use the default
		DDR3 configuration.

Diff:

Revision 3 (+147 -148)

Show changes

	tests/configs/base_config.py
	tests/configs/inorder-timing.py
	tests/configs/o3-timing.py
	tests/configs/o3-timing-checker.py
	tests/configs/simple-atomic.py
	tests/configs/simple-atomic-dummychecker.py
	tests/configs/simple-timing.py
	tests/long/se/10.mcf/test.py
	10 more

Description:

~		Changeset 9772:b914860111a9
	~	Changeset 9773:57a236cff5e9

		config: Add a BaseSESystem builder for re-use in regressions

		This patch extends the existing system builders to also include a
~		syscall-emulation base builder. To start with, this builder is only
~		deployed in a limited number of cases, most notably o3-timing,
~		simple-timing and simple-atomic. The main reason for this is to not
~		change any regressions.
	~	syscall-emulation builder. This builder is deployed in all
	~	syscall-emulation regressions that do not involve Ruby,
	~	i.e. o3-timing, simple-timing and simple-atomic, as well as the
	~	multi-processor regressions o3-timing-mp, simple-timing-mp and
	+	simple-atomic-mp (the latter are only used by SPARC at this point).

~		At this point only a uni-processor builder is added, and the values
~		chosen for the cache sizes match those that were used in the existing
~		config scripts (despite being on the large side).
	~	The values chosen for the cache sizes match those that were used in
	~	the existing config scripts (despite being on the large
	~	side). Similarly, a mem_class parameter is added to the builder base
	+	class to enable simple-atomic to use SimpleMemory and o3-timing to use
	+	the default DDR3 configuration.

~		A mem_class parameter is added to the builder base class to enable
~		simple-atomic to use SimpleMemory and o3-timing to use the default
~		DDR3 configuration.
	~	Due to the different order the ports are connected, the bus stats get
	~	shuffled around for the multi-processor regressions. A separate patch
	~	bumps the port indices. Besides this, all behaviour is exactly the
	+	same.

Diff:

Revision 4 (+195 -274)

Show changes

	tests/configs/base_config.py
	tests/configs/inorder-timing.py
	tests/configs/o3-timing.py
	tests/configs/o3-timing-checker.py
	tests/configs/o3-timing-mp.py
	tests/configs/simple-atomic.py
	tests/configs/simple-atomic-dummychecker.py
	tests/configs/simple-atomic-mp.py
	13 more

You have a pending review.

Review Board 2.0.15

This change has been marked as submitted.

Screenshots

Files

Issue Summary

Description:

Changeset 9757:47d39456a0ec

Changeset 9772:b3272cd98ff7

Diff:

Description:

Changeset 9772:b3272cd98ff7

Changeset 9772:b914860111a9

Diff:

Description:

Changeset 9772:b914860111a9

Changeset 9773:57a236cff5e9

Diff:

Status: Closed (submitted)