-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://reviews.gem5.org/r/3565/#review8489
-----------------------------------------------------------
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 (line 102)
<http://reviews.gem5.org/r/3565/#comment7350>
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 (line 94)
<http://reviews.gem5.org/r/3565/#comment7357>
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 (line 95)
<http://reviews.gem5.org/r/3565/#comment7353>
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 (line 103)
<http://reviews.gem5.org/r/3565/#comment7351>
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 (line 105)
<http://reviews.gem5.org/r/3565/#comment7352>
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 (line 141)
<http://reviews.gem5.org/r/3565/#comment7354>
Remove if unneeded.
configs/example/arm/fs_bigLITTLE.py (line 158)
<http://reviews.gem5.org/r/3565/#comment7355>
Why not make this a member function of SimpleSystem? Or inheirit from
SimpleSystem to create a CachedSystem.
configs/example/arm/fs_bigLITTLE.py (line 217)
<http://reviews.gem5.org/r/3565/#comment7356>
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.
- Jason Lowe-Power
On July 12, 2016, 7:16 a.m., Andreas Sandberg wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://reviews.gem5.org/r/3565/
> -----------------------------------------------------------
>
> (Updated July 12, 2016, 7:16 a.m.)
>
>
> Review request for Default.
>
>
> Repository: gem5
>
>
> Description
> -------
>
> Changeset 11567:f22241c1afcc
> ---------------------------
> arm, config: Add an example ARM big.LITTLE(tm) configuration script
>
> An ARM big.LITTLE system consists of two cpu clusters: the big
> CPUs are typically complex out-of-order cores and the little
> CPUs are simpler in-order ones. The fs_bigLITTLE.py script
> can run a full system simulation with various number of big
> and little cores and cache hierarchy. The commit also includes
> two example device tree files for booting Linux on the
> bigLITTLE system.
>
> Change-Id: I6396fb3b2d8f27049ccae49d8666d643b66c088b
> Reviewed-by: Andreas Sandberg <andreas.sandb...@arm.com>
>
>
> Diffs
> -----
>
> configs/example/arm/fs_bigLITTLE.py PRE-CREATION
> system/arm/dt/Makefile f050b8cf4754
> system/arm/dt/armv8_big_little.dts PRE-CREATION
> configs/example/arm/devices.py PRE-CREATION
>
> Diff: http://reviews.gem5.org/r/3565/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Andreas Sandberg
>
>
_______________________________________________
gem5-dev mailing list
gem5-dev@gem5.org
http://m5sim.org/mailman/listinfo/gem5-dev
