> On May 4, 2014, 4:56 p.m., Steve Reinhardt wrote: > > src/python/m5/params.py, line 84 > > <http://reviews.gem5.org/r/2246/diff/1/?file=39625#file39625line84> > > > > this seems clunky... would it be easier to list the classes that > > *can't* be set on the command line? > > Geoffrey Blake wrote: > 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.
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. > On May 4, 2014, 4:56 p.m., Steve Reinhardt wrote: > > src/python/m5/simulate.py, line 117 > > <http://reviews.gem5.org/r/2246/diff/1/?file=39626#file39626line117> > > > > 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. > > Steve Reinhardt wrote: > BTW, is this change independent of the command-line parameter stuff? If > so, then it should be a separate changeset. > > Geoffrey Blake wrote: > 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 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. > On May 4, 2014, 4:56 p.m., Steve Reinhardt wrote: > > src/python/m5/SimObject.py, line 654 > > <http://reviews.gem5.org/r/2246/diff/1/?file=39624#file39624line654> > > > > 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. > > Geoffrey Blake wrote: > 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 > > > 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? > On May 4, 2014, 4:56 p.m., Steve Reinhardt wrote: > > src/python/m5/SimObject.py, line 597 > > <http://reviews.gem5.org/r/2246/diff/1/?file=39624#file39624line597> > > > > Should it live in params.py? Seems like that would be more > > appropriate. Can it be folded in with ParamDesc? > > Geoffrey Blake wrote: > 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. Hi Geoff, thanks for the responses. I see what you're saying, this placement seems reasonable. - Steve ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: http://reviews.gem5.org/r/2246/#review5082 ----------------------------------------------------------- On April 23, 2014, 5:22 a.m., Andreas Hansson wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > http://reviews.gem5.org/r/2246/ > ----------------------------------------------------------- > > (Updated April 23, 2014, 5:22 a.m.) > > > Review request for Default. > > > Repository: gem5 > > > Description > ------- > > Changeset 10198:f749128e94c8 > --------------------------- > config: Add hooks to enable new config sys > > This patch adds helper functions to SimObject.py, params.py and > simulate.py to enable the new configuration system. Functions like > enumerateParams() in SimObject lets the config system auto-generate > command line options for simobjects to be modified on the command > line. > > Params in params.py have __call__() added > to their definition to allow the argparse module to use them > as a type to check command input is in the proper format. > > simulate.py is changed to allow for empty simobjects to exist in the hierarchy > to allow a system to be built in parts to be stitched together later. > > > Diffs > ----- > > src/python/m5/SimObject.py e40b35147270 > src/python/m5/params.py e40b35147270 > src/python/m5/simulate.py e40b35147270 > src/python/m5/stats/__init__.py e40b35147270 > > Diff: http://reviews.gem5.org/r/2246/diff/ > > > Testing > ------- > > > Thanks, > > Andreas Hansson > > _______________________________________________ gem5-dev mailing list gem5-dev@gem5.org http://m5sim.org/mailman/listinfo/gem5-dev
