----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: http://reviews.gem5.org/r/3683/#review8944 -----------------------------------------------------------
Ship it! This is fine with me. However, I think this is just bandaging a broken system. IMO, we need to greatly simplify (maybe even totally remove) the command-line parameters in config/common. What I've been telling people is this: gem5's interface is not the command line: it is Python scripts. You shouldn't think about "what options should I pass to gem5" when you are using it, you should instead say "what Python script should I write". My opinion is our scripts in configs/ should do two things: 1. Break out commonalities between all Python configuration scripts. Things like DDR3 memory, basic cache coherence protocols, specific ARM CPU configurations, full system setup BS, etc. 2. Give some basic examples of how to use Python configuration scripts, and give some starting points for new users to pick up gem5. Sorry, this is off-topic for this patch, but it's something that's been bothering me for years now. This idea that many of our users have that you can just call gem5 with the "right" command line paramters to simulate what you need is harmful. It's harmful both to gem5 development as it makes these config scripts less maintainable (e.g., Ruby's scripts), and, in the worst case, produces bad architecture research since users can ignore important parameters. /rant. This isn't really a review of this patch, but I just wanted to say I don't think it's worth much of our time to try to bandage the current "Options.py" configuration style. Though, I'm not volunteering to fix this either. BTW, I think the ARM config scripts in configs/examples/arm are a step in the right direction, though I think they could be taken even further to make them cleaner and more maintainable. Either way, thanks for making these changes, Andreas. We certainly can't have things that are generating errors for our users in the short term. configs/common/Options.py (line 46) <http://reviews.gem5.org/r/3683/#comment7689> Weird. I guess I understand why this is needed, but it's super strange to have to import from the package this file is part of. Something else that should be solved someday, I guess. - Jason Lowe-Power On Oct. 20, 2016, 11:07 a.m., Andreas Hansson wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > http://reviews.gem5.org/r/3683/ > ----------------------------------------------------------- > > (Updated Oct. 20, 2016, 11:07 a.m.) > > > Review request for Default. > > > Repository: gem5 > > > Description > ------- > > Changeset 11688:60b7de6442f3 > --------------------------- > config: Break out base options for usage with NULL ISA > > This patch breaks out the most basic configuration options into a set > of base options, to allow them to be used also by scripts that do not > involve any actual CPUs or devices. The patch also fixes a few modules > so that they can be imported in a NULL build, and avoid dragging in > FSConfig every time Options is imported. > > > Diffs > ----- > > configs/common/CpuConfig.py b3d5f0e9e258 > configs/common/FSConfig.py b3d5f0e9e258 > configs/common/Options.py b3d5f0e9e258 > configs/common/PlatformConfig.py b3d5f0e9e258 > configs/example/garnet_synth_traffic.py b3d5f0e9e258 > > Diff: http://reviews.gem5.org/r/3683/diff/ > > > Testing > ------- > > > Thanks, > > Andreas Hansson > > _______________________________________________ gem5-dev mailing list [email protected] http://m5sim.org/mailman/listinfo/gem5-dev
