Overall, things look pretty good. There are more or less four things
that you've done that need work.
1) You created SimObjects for all of the ruby objects and in the
process, you didn't give any help strings to any of the parameters.
While I understand that this can be tedious, this is one minimal form
of documentation that does help users.
2) You removed Garnet from the compile. Do we want to keep garnet?
If so, this is guaranteed to lead to bit rot.
3) You commented out code instead of removing it. This should only
*very* rarely be done. The old code is in the history, so just delete
it.
4) You've added #includes in places, but have not maintained sorted
order. It's a lot easier to visually scan #includes if they're all
sorted. This lets you easily find duplicates and quickly figure out
if you need to add something. (There's something in the coding style
document about this.)
I also have several comments inline.
Nate
> +### create the desired simulated system
> +### ruby memory must be at least 16 MB to work with the mem tester
> +##ruby_memory = ruby_config.generate("MI_example-homogeneous.rb",
> +## cores = options.testers,
> +## memory_size = 16,
> +## ports_per_cpu = 1)
Please don't comment out code and then commit it. It's in the
history, so please just delete it.
> -system = System(cpu = cpus, funcmem = PhysicalMemory(),
> - physmem = ruby_memory)
> +system = System(cpu = cpus,
> + funcmem = PhysicalMemory(),
> + physmem = PhysicalMemory())
Do you really want two different PhysicalMemory objects here?
> diff -r 289ac904233d -r 8f73bf7c3c5e src/mem/ruby/common/Debug.hh
> --- a/src/mem/ruby/common/Debug.hh Wed Nov 18 18:00:41 2009 -0800
> +++ b/src/mem/ruby/common/Debug.hh Sat Dec 12 14:37:14 2009 -0800
> #include "config/ruby_debug.hh"
> #include "mem/ruby/common/Global.hh"
> +#include "sim/sim_object.hh"
> +
> +#include "params/RubyDebug.hh"
Please keep all includes in a single block and sort them. This makes
it much easier to find an include and to avoid duplicate includes.
There is a section in the style guide about this.
> diff -r 289ac904233d -r 8f73bf7c3c5e src/mem/ruby/libruby.cc
> --- a/src/mem/ruby/libruby.cc Wed Nov 18 18:00:41 2009 -0800
> +++ b/src/mem/ruby/libruby.cc Sat Dec 12 14:37:14 2009 -0800
> +#if 0
> void libruby_init(const char* cfg_filename)
> {
> ifstream cfg_output(cfg_filename);
> @@ -118,6 +119,7 @@
> RubySystem::create(*sys_conf);
> delete sys_conf;
> }
> +#endif
This seems wrong. Either the code should just be removed, or things
should be initialized in some new way. I'm guessing the latter.
> diff -r 289ac904233d -r 8f73bf7c3c5e
> src/mem/ruby/network/garnet-fixed-pipeline/GarnetNetwork_d.cc
> --- a/src/mem/ruby/network/garnet-fixed-pipeline/GarnetNetwork_d.cc Wed
> Nov 18 18:00:41 2009 -0800
> +++ b/src/mem/ruby/network/garnet-fixed-pipeline/GarnetNetwork_d.cc Sat
> Dec 12 14:37:14 2009 -0800
> -void GarnetNetwork_d::init(const vector<string> & argv)
> +void GarnetNetwork_d::init()
> {
> Network::init(argv);
>
This must not compile. You've removed argv, yet you still use it.
> diff -r 289ac904233d -r 8f73bf7c3c5e
> src/mem/ruby/network/garnet-fixed-pipeline/SConscript
> --- a/src/mem/ruby/network/garnet-fixed-pipeline/SConscript Wed Nov 18
> 18:00:41 2009 -0800
> +++ b/src/mem/ruby/network/garnet-fixed-pipeline/SConscript Sat Dec 12
> 14:37:14 2009 -0800
> +# temporarily disable
> +Return()
> +
You shouldn't commit this. Seems like you need to fix garnet.
> diff -r 289ac904233d -r 8f73bf7c3c5e
> src/mem/ruby/network/garnet-flexible-pipeline/SConscript
> --- a/src/mem/ruby/network/garnet-flexible-pipeline/SConscript Wed Nov 18
> 18:00:41 2009 -0800
> +++ b/src/mem/ruby/network/garnet-flexible-pipeline/SConscript Sat Dec 12
> 14:37:14 2009 -0800
> +# temporarily disable
> +Return()
> +
Again
> diff -r 289ac904233d -r 8f73bf7c3c5e src/mem/ruby/network/orion/SConscript
> --- a/src/mem/ruby/network/orion/SConscript Wed Nov 18 18:00:41 2009 -0800
> +++ b/src/mem/ruby/network/orion/SConscript Sat Dec 12 14:37:14 2009 -0800
> +# temporarily disable
> +Return()
> +
Again
> diff -r 289ac904233d -r 8f73bf7c3c5e src/mem/ruby/system/CacheMemory.cc
> --- a/src/mem/ruby/system/CacheMemory.cc Wed Nov 18 18:00:41 2009 -0800
> +++ b/src/mem/ruby/system/CacheMemory.cc Sat Dec 12 14:37:14 2009 -0800
> +#if 0
> + for (uint32 i=0; i<argv.size(); i+=2) {
> + if (m_last_level_machine_type < m_controller->getMachineType()) {
> + m_num_last_level_caches =
> + MachineType_base_count(m_controller->getMachineType());
> + m_last_level_machine_type =
> + m_controller->getMachineType();
> + }
> + }
> +#endif
I assume that this should be deleted?
> +void Sequencer::init()
> +{
> }
I just noticed this, and I'm not sure if you do it elsewhere, but it
seems that you should just remove the init() function in cases where
it is empty.
> diff -r 289ac904233d -r 8f73bf7c3c5e src/mem/ruby/system/Sequencer.hh
> --- a/src/mem/ruby/system/Sequencer.hh Wed Nov 18 18:00:41 2009 -0800
> +++ b/src/mem/ruby/system/Sequencer.hh Sat Dec 12 14:37:14 2009 -0800
> -class Sequencer : public Consumer, public RubyPort {
> +class Sequencer : public RubyPort, public Consumer {
Is MI really necessary? In general, we avoid it if we can because it
can be confusing. (I know that you didn't add it, so you don't need
to fix it here) I'm also curious, is there a reason that order
mattered? For my own education, I'd like to understand what was going
on if it did.
> diff -r 289ac904233d -r 8f73bf7c3c5e src/mem/ruby/system/System.cc
> --- a/src/mem/ruby/system/System.cc Wed Nov 18 18:00:41 2009 -0800
> +++ b/src/mem/ruby/system/System.cc Sat Dec 12 14:37:14 2009 -0800
> -#include "mem/ruby/network/garnet-flexible-pipeline/GarnetNetwork.hh"
> -#include "mem/ruby/network/garnet-fixed-pipeline/GarnetNetwork_d.hh"
> +//#include "mem/ruby/network/garnet-flexible-pipeline/GarnetNetwork.hh"
> +//#include "mem/ruby/network/garnet-fixed-pipeline/GarnetNetwork_d.hh"
Should get fixed
> @@ -146,6 +110,7 @@
> * e.g. a sequencer needs a pointer to a controller and a controller needs
> a pointer to a sequencer
> */
>
> +#if 0
> vector<string> memory_control_names;
>
> for (size_t i=0;i<sys_conf.size(); i++) {
Delete the code, don't comment it.
> @@ -153,32 +118,32 @@
> const string & name = sys_conf[i].name;
> if (type == "System" || type == "Debug")
> continue;
> - else if (type == "SetAssociativeCache")
> - m_caches[name] = new CacheMemory(name);
> - else if (type == "DirectoryMemory")
> - m_directories[name] = new DirectoryMemory(name);
> - else if (type == "Sequencer") {
> - m_sequencers[name] = new Sequencer(name);
> - m_ports[name] = m_sequencers[name];
> - } else if (type == "DMASequencer") {
> - m_dma_sequencers[name] = new DMASequencer(name);
> - m_ports[name] = m_dma_sequencers[name];
> + // else if (type == "SetAssociativeCache")
> + // m_caches[name] = new CacheMemory(name);
> +// else if (type == "DirectoryMemory") {
> +// m_directories[name] = new DirectoryMemory(name);
> +// else if (type == "Sequencer") {
> +// m_sequencers[name] = new Sequencer(name);
> +// m_ports[name] = m_sequencers[name];
> +// } else if (type == "DMASequencer") {
> +// m_dma_sequencers[name] = new DMASequencer(name);
> +// m_ports[name] = m_dma_sequencers[name];
> } else if (type == "Topology") {
> assert(m_topologies.size() == 0); // only one toplogy at a time is
> supported right now
> m_topologies[name] = new Topology(name);
> } else if (type == "SimpleNetwork") {
> assert(m_network_ptr == NULL); // only one network at a time is
> supported right now
> m_network_ptr = new SimpleNetwork(name);
> - } else if (type.find("generated") == 0) {
> - string controller_type = type.substr(10);
> - m_controllers[name] =
> ControllerFactory::createController(controller_type, name);
> +// } else if (type.find("generated") == 0) {
> +// string controller_type = type.substr(10);
> +// m_controllers[name] =
> ControllerFactory::createController(controller_type, name);
> // printf ("ss: generated %s \n", controller_type);
> //added by SS
> - } else if (type == "Tracer") {
> +// } else if (type == "Tracer") {
> //m_tracers[name] = new Tracer(name);
> - m_tracer_ptr = new Tracer(name);
> - } else if (type == "Profiler") {
> - m_profiler_ptr = new Profiler(name);
> +// m_tracer_ptr = new Tracer(name);
> +// } else if (type == "Profiler") {
> +// m_profiler_ptr = new Profiler(name);
> } else if (type == "GarnetNetwork") {
> assert(m_network_ptr == NULL); // only one network at a time is
> supported right now
> m_network_ptr = new GarnetNetwork(name);
> @@ -255,9 +220,12 @@
> else
> assert(0);
> }
> +#endif
> +}
Seems that all of this should just be deleted
_______________________________________________
m5-dev mailing list
m5-dev@m5sim.org
http://m5sim.org/mailman/listinfo/m5-dev
