Overall this looks pretty interesting.  Some minor quibbles below, but no major objections (other than the 'if hasattr()' stuff at the end).  I would like to see some examples of what a command-line setting looks like though to help me understand the enumerateParams() method and the ParamInfo class a little better though.

src/python/m5/SimObject.py (Diff revision 1)

comment please?  i don't understand the condition here, or why it's elif and not a separate if

src/python/m5/SimObject.py (Diff revision 1)

Should it live in params.py?  Seems like that would be more appropriate.  Can it be folded in with ParamDesc?  

1. Geoffrey Blake June 26, 2014, 7:48 a.m. (June 26, 2014, 7:48 a.m.)

   Hi Steve, I wrote this patch and can comment on your questions.  This could be folded into ParamDesc without much effort, but I think it may be clearer to keep this to better explain what the enumerateParams() function below is attempting to accomplish.

2. Steve Reinhardt July 1, 2014, 5:45 p.m. (July 1, 2014, 5:45 p.m.)

   Hi Geoff, thanks for the responses.  I see what you're saying, this placement seems reasonable.

src/python/m5/SimObject.py (Diff revision 1)

This seems kind of complicated, but I'm going to wait for a higher-level description of what it's doing before I dig into it.

1. Geoffrey Blake June 26, 2014, 7:48 a.m. (June 26, 2014, 7:48 a.m.)

   The intent is that you can build a configuration system that exposes all params to be settable on the command line (instead of the tiny fraction available now) and a system that auto-updates its available settable command line flags whenever a simobject's params are edited.  The other intention of this function is to provide the argument parser with enough information about each parameter to make unique command lines, and also use the ParamValue itself as an input check to catch any errors in specifying a command as early as possible.
   
   The enumerateParams() function takes a simobject instance and traces out all the reachable parameters it contains and constructs information to be presented on the command line.  For example, if you passed an instance of O3_ARM_v7a_3 to enumerateParams() it will generate command lines to be presented such as:
   --testsys-cpucluster-O3_ARM_v7a_3.branchPred.BTBTagSize
   --testsys-cpucluster-O3_ARM_v7a_3.fuPool.FUList0.count
   --testsys-cpucluster-O3_ARM_v7a_3.LQEntries
   
   
   

2. Steve Reinhardt July 1, 2014, 5:45 p.m. (July 1, 2014, 5:45 p.m.)

   OK, I see.  So I take it there's some additional code needed in se.py/fs.py/etc. to make this work?  Would it make sense to include that in this patch, so people can see how to use it?

3. Geoffrey Blake July 10, 2014, 3:04 p.m. (July 10, 2014, 3:04 p.m.)

   There is a large chunk code that is a full top to bottom re-write of the mainline configuration scripts, so it wouldn't make much sense to include in this patch.

4. Steve Reinhardt July 10, 2014, 8:22 p.m. (July 10, 2014, 8:22 p.m.)

   OK.  Are you intending to commit that separately then?

5. Ali Saidi July 10, 2014, 9:04 p.m. (July 10, 2014, 9:04 p.m.)

   Yes, but the timeline is tbd.

src/python/m5/SimObject.py (Diff revision 1)

this comment seems 90% redundant with the one above... if you just move the one line of code up under this one:
  self._values = multidict(ancestor._values)
then I think it would be self-explanatory

src/python/m5/SimObject.py (Diff revision 1)

this obviously parallels the if/elif above, except for the proxy check (is it correct to skip that here?)... and like above, it could use a comment

src/python/m5/params.py (Diff revision 1)

this seems clunky... would it be easier to list the classes that *can't* be set on the command line?

1. Geoffrey Blake June 26, 2014, 7:48 a.m. (June 26, 2014, 7:48 a.m.)

   I did it this way as a guard against adding new Params that may not be able to be spec'ed on the command line and have the enumerateParams() function skip it as the default behavior.  Flipping it to be a list of what not to trace could cause things to go haywire if a new Param type was added that was missing functionality and the developer forgot to put it in the "do not trace this" blacklist.

2. Steve Reinhardt July 1, 2014, 5:45 p.m. (July 1, 2014, 5:45 p.m.)

   I see. Looking at this again, I think a better solution would be to have a class static variable like isCommandLineSettable, make a default value in SimObject of False, and then override it to True in the subclasses that are settable.

3. Geoffrey Blake July 10, 2014, 3:04 p.m. (July 10, 2014, 3:04 p.m.)

   That would be a better option.

src/python/m5/params.py (Diff revision 1)

how does printing the entire map here help?  self.vals is the list of keys, I believe, which should be all that's relevant for this error message

src/python/m5/simulate.py (Diff revision 1)

for all these 'if hasattr()' changes (here and below, and in simulate.py): seems like it would be much cleaner to make sure whatever dummy objects are being inserted into the config tree support dummy versions of these calls rather than add all these distributed checks all over the place. Maybe create a special DummySimObject class or something?  I don't fully understand the use case here or perhaps I could be more specific.

1. Steve Reinhardt May 4, 2014, 11:03 p.m. (May 4, 2014, 11:03 p.m.)

   BTW, is this change independent of the command-line parameter stuff?  If so, then it should be a separate changeset.

2. Geoffrey Blake June 26, 2014, 7:48 a.m. (June 26, 2014, 7:48 a.m.)

   Internally we had this discussion and it was deemed a worse maintenance headache having a DummySimobject C++ class over being handled purely in the python code.  It can be a separate changeset as well

3. Steve Reinhardt July 1, 2014, 5:45 p.m. (July 1, 2014, 5:45 p.m.)

   You didn't invite me to your discussion ;).  It seems to me like we've established what the SimObject interface is, and we've already added dummy versions of these methods to the base SimObject class so that we don't have to put all these tests in, even though many derived classes don't implement them.  So a DummySimObject class ought to be pretty trivial, and putting in all these tests now seems like a step backwards to me.
   
   As I said initially, I don't really understand the use case for these "dummy objects" anyway, or how you can call simulate() when you don't have all the objects in place yet, so maybe I'm missing something, but so far I remain unconvinced.

4. Geoffrey Blake July 10, 2014, 3:04 p.m. (July 10, 2014, 3:04 p.m.)

   The dummy simobject is being used as a means to provide naming resolution only.  In the rewrite of the config scripts, it was a desired feature to have functions that created whole subsystems that we could call multiple times to instantiate the desired system:
   
   cluster1 = makeCpuAndCaches()
   cluster2 = makeCpuAndCaches()
   
   SimObjects cluster1 and 2 would have identical sets of Simobjects as children, but their final names would be unique (cluster1.cpu0.icache, cluster2.cpu0.icache etc).  In the end Objects cluster1 and cluster2 aren't instantiated (I have forgotten how now), but the simulated system still works correctly because we made sure to connect up all the ports properly.  Making an actual DummySimobject class is not hard, I did that in a previous iteration.

5. Ali Saidi July 10, 2014, 3:13 p.m. (July 10, 2014, 3:13 p.m.)

   My reasoning here was that we didn't really want to have a sim object that was instantiated by did nothing, wasn't connected to anything and only existed to provide containers for configuration. 

6. Steve Reinhardt July 10, 2014, 8:22 p.m. (July 10, 2014, 8:22 p.m.)

   That argument sounds reasonable on its own, but I feel like the consequences of following that path ultimately make it the least objectionable alternative.  (IMO, being less objectionable than sprinkling 'hasattr' conditionals all over is a pretty low bar.)
   
   It would be nice to avoid creating the useless C++ object if we could have the python object support the full SimObject interface, but that won't work because the python SimObject actually derives the interface dynamically from the C++ version (see SimObject.__getattr__).  So the C++ object isn't totally useless, in that it is serving as a proxy to export the C++ SimObject API out through the python code.
   
   Also, if the purpose is to serve as a container, then a name like Container or SimObjectContainer seems more appropriate.

7. Geoffrey Blake July 25, 2014, 8:09 p.m. (July 25, 2014, 8:09 p.m.)

   Hi Steve,
   
   I've updated this patch internally with the suggested edits and split it into two. They should be posted early monday to the reviewboard. I did go forward and re-create the container simobject as a C++ object and called it SubSystem.  

Any final comments on this one and http://reviews.gem5.org/r/2318/?

Ship It!