On Fri, Sep 11, 2009 at 7:57 PM, nathan binkert <[email protected]> wrote:
> I finally read this diff. My comments are inline. In the future,
> please use "hg email" which is found in the patchbomb extension. It
> makes it far easier to comment on diffs since I can just then reply to
> the e-mail without having to jump through hoops to get it into my
> mailer.
>
> Nate
>
Thanks for reading it and will do.
>
> > diff -r 369b61762d7b -r e5a3ba0bbe88 tests/configs/memtest-ruby.py
> > --- a/tests/configs/memtest-ruby.py Mon Aug 31 16:38:22 2009 -0500
> > +++ b/tests/configs/memtest-ruby.py Mon Aug 31 16:39:59 2009 -0500
> > @@ -29,13 +29,15 @@
> > import m5
> > from m5.objects import *
> >
> > +#MAX CORES IS 8 with the fals sharing method
> typo: false
>
> > +assert(nb_cores > 0 and nb_cores <= 8)
> > +assert(nb_cores == int(nb_cores))
> > +assert(config_file != 'none')
> >
> > -#MAX CORES IS 8 with the fals sharing method
> > -nb_cores = 8
> > cpus = [ MemTest() for i in xrange(nb_cores) ]
> >
> > import ruby_config
> > -ruby_memory = ruby_config.generate("MI_example-homogeneous.rb",
> nb_cores)
> > +ruby_memory = ruby_config.generate(config_file, nb_cores)
> >
> > # system simulated
> > system = System(cpu = cpus, funcmem = PhysicalMemory(),
> why did you move nb_cores to run.py? I think that's the wrong place
> for that. run.py is supposed to only run the script, nothing more.
> If you want some place to stick ruby stuff, create another file that
> all ruby tests can import.
>
>
I moved it to run.py because it is effectively the parameter that I am
passing to the script when I invoke it. I can change that so run.py is
untouched.
>
> > diff -r 369b61762d7b -r e5a3ba0bbe88 tests/quick/50.memtest/test.py
> > --- a/tests/quick/50.memtest/test.py Mon Aug 31 16:38:22 2009 -0500
> > +++ b/tests/quick/50.memtest/test.py Mon Aug 31 16:39:59 2009 -0500
> > @@ -25,6 +25,8 @@
> > # OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
> > #
> > # Authors: Ron Dreslinski
> > +import m5
> > +from m5.objects import *
> >
> > MemTest.max_loads=1e5
> > MemTest.progress_interval=1e4
> Is this actually necessary?
>
>
> > diff -r 369b61762d7b -r e5a3ba0bbe88 tests/run.py
> > --- a/tests/run.py Mon Aug 31 16:38:22 2009 -0500
> > +++ b/tests/run.py Mon Aug 31 16:39:59 2009 -0500
> > @@ -38,6 +38,7 @@
> >
> > # single "path" arg encodes everything we need to know about test
> > (category, name, isa, opsys, config) = sys.argv[1].split('/')[-5:]
> > +(build, protocol) = sys.argv[1].split('/')[:2]
> There's a better way to do this below.
>
> >
> > # find path to directory containing this file
> > tests_root = os.path.dirname(__file__)
> > @@ -62,7 +63,15 @@
> >
> > # build configuration
> > sys.path.append(joinpath(tests_root, 'configs'))
> > -execfile(joinpath(tests_root, 'configs', config + '.py'))
> > +nb_cores = 8
> > +config_file = 'none'
> > +if (protocol == 'LIBRUBY_MI_example'):
> > + config_file = 'MI_example-homogeneous.rb'
> > +elif (protocol == 'LIBRUBY_MOESI_CMP_directory'):
> > + config_file = 'TwoLevel_SplitL1UnifiedL2.rb'
> > +
> > +globals = {'nb_cores':nb_cores, 'config_file':config_file}
> The added code above should go into some sort of Ruby file. Also the
> protocol variable is just something that you guys did and isn't how
> the protocol is really stored. The right way to do it is add
> 'PROTOCOL' to export_vars in src/mem/protocol/SConsopts (See
> src/mem/ruby/SConsopts for an example of how this is done.) You can
> then use m5.build_env['PROTOCOL'] to get the actual protocol name.
> (just grep for build_env in the tree to see examples).
>
> > +execfile(joinpath(tests_root, 'configs', config + '.py'), globals,
> globals)
> Given that you shouldn't be sticking nb_cores and config_file here, I
> don't think you should change this line.
>
Got it. I will try again.
> _______________________________________________
> m5-dev mailing list
> [email protected]
> http://m5sim.org/mailman/listinfo/m5-dev
>
_______________________________________________
m5-dev mailing list
[email protected]
http://m5sim.org/mailman/listinfo/m5-dev
