arm, config: Fixups for the example big.LITTLE(tm) configuration
Review Request #3565 - Created July 12, 2016 and submitted
| Information | |
|---|---|
| Andreas Sandberg | |
| gem5 | |
| default | |
| Reviewers | |
| Default | |
Changeset 11607:5f734e717904 --------------------------- arm, config: Fixups for the example big.LITTLE(tm) configuration This patch refactors the configuration file to use a more object-oriented design. Change-Id: I44ac2d063c2b5901f385544fb6ce3f259459cb05 Reviewed-by: Andreas Sandberg <andreas.sandberg@arm.com> Reviewed-by: Gabor Dozsa <gabor.dozsa@arm.com>
Issue Summary
I really like how these new config files are nice and encapsulated! I think this is the way forward for our config system.
I have mostly minor comments below about a few ways to clean this up even more and use a more object-oriented design. Most comments can be ignored if you feel strongly against them.
My biggest comment is that you should be using class constructors more. Just FYI, I've found (through trial and error) that you have to always have the following for SimObjects to work right (e.g., for SimpleSystem):
class SimpleSystem(<super class>): def __init__(self, <other args>): super(SimpleSystem, self).__init__() <the rest of your constructor>
-
configs/example/arm/devices.py (Diff revision 1) -
I think some of these lines should be in an
__init__function instead of static class members.IMO, only default values (e.g., cache_line_size) should be members of the class object. Everything else should be members of the instantiated objects. Do you feel strongly in a different way?
I think basically everything here (except maybe the cache_line_size) should be in the constructor.Although I doubt it would cause any issues today, I believe if you were to instantiate two "SimpleSystem" objects, they would end up sharing many of the same "pointers" to other objects.
-
configs/example/arm/fs_bigLITTLE.py (Diff revision 1) -
Personally, I would make two subclasses of CPUCluster: BigCluster and LittleCluster. Then you could drop this confusing "cpu_config" parameter and just use defaults in each subclass.
But this isn't a big deal.
-
configs/example/arm/fs_bigLITTLE.py (Diff revision 1) -
Could you add some documentation here? What is the type of cpu_config, etc.? Ignore this if you take the above advise.
-
configs/example/arm/fs_bigLITTLE.py (Diff revision 1) -
How do you know that the system is your parent? At a minimum you should assert the type is correct. However, I think the system should be a parameter to the CpuCluster constructor.
-
configs/example/arm/fs_bigLITTLE.py (Diff revision 1) -
IMO, this is a good place to use a wrapper function in SimpleSystem, but it's up to you. It's not great practice to directly access members of other object, but Python lets you do anything so...
-
configs/example/arm/fs_bigLITTLE.py (Diff revision 1) -
Remove if unneeded.
-
configs/example/arm/fs_bigLITTLE.py (Diff revision 1) -
Why not make this a member function of SimpleSystem? Or inheirit from SimpleSystem to create a CachedSystem.
-
configs/example/arm/fs_bigLITTLE.py (Diff revision 1) -
Why did you use "CpuConfig.get()" here and just used the class (MinorCPU) below. Is there some extra logic in CpuConfig.get? If it always returns the same class you might as well just use it explicitly.
Summary: |
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Description: |
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Diff: |
Revision 2 (+181 -147) |
Thanks for the changes. It looks much better. This is now a good example of how the configs should be used, IMO.
