Brad
* The functions for adding options introduced by the patch could have been
placed in se.py and fs.py. I did not do that because ruby_fs.py and fs.py
have options in common, and to avoid duplication those options were placed
in Options.py. And the options in se.py were also moved to Options.py so
that a common file has all the options in it. We can always move these
functions back in to their respective files.
* Ruby's options are being added separately via function calls, so I did
not see any need to change it. I certainly feel that a lot of duplication
exists across scripts in configs/ruby directory, but it would require some
thinking on how to combine the scripts together.
* These configuration files are examples which can be used to write config
files for a specific need. I don't know if the files in config/ change
more often that those in src/. But assuming that's true, any patches for
these files that users maintain will need to be rebased more often than
other patches. I suggest that users should maintain separate config files
instead of modifying se/fs.py, so that they are not affected.
* One of the things that I felt is worth spending time on, is to make
regression tests use se/fs.py by setting options properly instead of
redoing the work of se/fs.py. And I think it is with this aim that I
posted the patch.
--
Nilay
On Thu, 29 Mar 2012, Beckmann, Brad wrote:
The reason I protest to this change is because I feel it is a step in
the wrong direction. As Steve says, the biggest pain point for us to
maintain patches that work on top of gem5 is the configuration scripts.
This changeset would centralize several system specific options into
Option.py which will likely cause unnecessary conflicts when future
parameters are added and seems to be contrast to the desire for more
modularity. Furthermore, the changeset uses a confusing policy where
most system specific parameters are put in Options.py but Ruby protocol
options still remain in their respective py config files. Finally,
there are a few minor semantic changes in this changeset that just don't
seem really worth it.
Sorry to push back so much on this, but my recent experience with
rebasing config files have made me leery of these sorts of changes.
Brad
From: Steve Reinhardt [mailto:[email protected]]
Sent: Wednesday, March 28, 2012 5:13 PM
To: gem5 Developer List
Cc: Nilay Vaish; Beckmann, Brad; Gabe Black
Subject: Re: [gem5-dev] Review Request: Config: Change the way options are added
On Wed, Mar 28, 2012 at 4:11 PM, Gabe Black
<[email protected]<mailto:[email protected]>> wrote:
I'm assuming Steve and Ali have already weighed the pros and cons which is why
they were ok with it going in.
Just to weigh in on this point... I approved it because it looked
reasonable (i.e., not unreasonable), but I did not do an extensive
cost/benefit analysis. I didn't really think about the impact on
existing scripts.
I will mention that config script modularity is a real problem we have,
and it would be nice to see steps toward making things more modular
(without having lots of duplicate code). I don't think this change is a
step in the wrong direction, but I'm not sure if it's a step forward or
just sideways.
Steve
_______________________________________________
gem5-dev mailing list
[email protected]
http://m5sim.org/mailman/listinfo/gem5-dev
