Eventually it probably will make sense to combine these files, but I’m not convinced that we’ve reach that point yet. For instance, the only common options between the two files are the benchmark option and now the kernel and script options. All other options supported by fs.py are not supported by Ruby. I think there is some nice cleanliness achieved by keeping these files separate and I fear a lot of nasty nested conditional statements will result by combining them. Remember these files are the minimal baseline implementations and typically users build complexity on top of them.
I think the se.py and ruby_se.py combination made more sense because there was more code shared between the two and less difference between what Classic and Ruby supported. For instance, there is no equivalent FSConfig.py file that the se files shared thus there was a lot more common functionality that was pushed up to those top-level se files. Also se mode doesn’t support multiple systems connected together via Ethernet, which Classic supports but Ruby does not. Brad From: Nilay Vaish [mailto:[email protected]] Sent: Tuesday, August 09, 2011 7:35 PM To: Nilay Vaish; Beckmann, Brad; Default Subject: Re: Review Request: Config: Add ruby_fs.py's functionality to fs.py This is an automatically generated e-mail. To reply, visit: http://reviews.m5sim.org/r/818/ On August 9th, 2011, 9:07 a.m., Brad Beckmann wrote: I just have a couple minor edits to make to the comments. However, I wonder what is the real benefit for combining the two fs files. I don't believe there is a lot of duplicate code between the two. Most of the added options in fs.py are not supported/related to Ruby. Also with your change, the main body of fs.py is now a large if/else statement with the if being the functionality from ruby_fs.py and the else being the fs.py functionality. Overall, I feel this is an unnecessary change that will negatively impact those of us who have other internal patches that modify these files. Why do it? There are a couple of reasons. We should not have different set of options for ruby_fs.py and fs.py. Since we have already combined se.py and ruby_se.py, we should do the same for fs scripts as well. The main body of fs.py being a large if/else is either because I have not taken out all those things that are common, and that we still lack in the integration that Ruby and M5 should achieve. As far as internal patches are concerned, unless the patches can be updated, they should be used with the version with which they were created. Going ahead, we are only going to add more features, sooner or later these features will step on the code used in the patches. - Nilay On August 6th, 2011, 12:43 a.m., Nilay Vaish wrote: Review request for Default. By Nilay Vaish. Updated 2011-08-06 00:43:50 Description Config: Add ruby_fs.py's functionality to fs.py This patch combines fs.py and ruby_fs.py and removes ruby_fs.py. Using ruby with fs.py would require mentioning --ruby on the command line. This same as se.py. Diffs * configs/example/fs.py (7a9a7f2a3d46) * configs/example/ruby_fs.py (7a9a7f2a3d46) View Diff<http://reviews.m5sim.org/r/818/diff/> _______________________________________________ gem5-dev mailing list [email protected] http://m5sim.org/mailman/listinfo/gem5-dev
