> On May 7, 2016, 2:34 p.m., Andreas Hansson wrote: > > src/cpu/o3/dyn_inst_impl.hh, line 180 > > <http://reviews.gem5.org/r/3449/diff/3/?file=55307#file55307line180> > > > > Why this->?
Same as in rest of file: accessing member of base class of template class. > On May 7, 2016, 2:34 p.m., Andreas Hansson wrote: > > src/cpu/o3/dyn_inst_impl.hh, line 190 > > <http://reviews.gem5.org/r/3449/diff/3/?file=55307#file55307line190> > > > > This is intruiging. Every write operation is turned into a SWAP? This > > seems like it could potentially have a whole range of unintended side > > effects. As said in the comment above this line: "Prerequisite for using MEM_SWAP is, that it should not be treated any differently than a regular write, with the only difference being that the data being overwritten is returned with the response. AFAIK, this holds for classic and Ruby." If you trace the usage of MEM_SWAP/isSwap throughout the codebase, it is used like a write, with the only difference being that the overwritten value is returned in the response packet. I further verified this by running a normal benchmark with unconditionally turning every write into a MEM_SWAP and the results are identical. > On May 7, 2016, 2:34 p.m., Andreas Hansson wrote: > > src/sim/mcversi.hh, line 60 > > <http://reviews.gem5.org/r/3449/diff/3/?file=55310#file55310line60> > > > > odd indentation, and no comments It's aligned with the opening brace '(', using spaces. What is the problem here? > On May 7, 2016, 2:34 p.m., Andreas Hansson wrote: > > src/sim/mcversi.cc, line 696 > > <http://reviews.gem5.org/r/3449/diff/3/?file=55311#file55311line696> > > > > Please double check that this works with gcc 4.8 and clang 3.1 GCC 4.8 is fine. Clang 3.1 I can't get my hands on, but if it supports C++11 there shouldn't be any problems. > On May 7, 2016, 2:34 p.m., Andreas Hansson wrote: > > src/sim/mcversi.cc, line 390 > > <http://reviews.gem5.org/r/3449/diff/3/?file=55311#file55311line390> > > > > Should these not just be removed rather? They have intentionally been left, so as to make it easier for whoever wants to use this. They are called in other parts of the code, and the call-sites cannot be changed: leaving them here ensures whoever adds coverage computation for their design updates coverage at the right points. > On May 7, 2016, 2:34 p.m., Andreas Hansson wrote: > > src/sim/mcversi.cc, line 374 > > <http://reviews.gem5.org/r/3449/diff/3/?file=55311#file55311line374> > > > > Is there any chance we could use the random_mt used throughout the rest > > of gem5 rather? We have had a number of cleanups to remove sporadic use of > > other forms of random number generation, and it would be good to not > > introduce new ones This is intentional, as using a global RNG would mean that test generation is not predictable if the system changes. At least this way, I know that my initial population is always going to be the same, even if the system has changed (e.g. when fixing a bug). > On May 7, 2016, 2:34 p.m., Andreas Hansson wrote: > > src/cpu/o3/commit_impl.hh, line 1269 > > <http://reviews.gem5.org/r/3449/diff/3/?file=55305#file55305line1269> > > > > Should this perhaps be a probe point? This is a bigger, and IMHO unnecessary change. Since it would turn into a SimObject, it would need to be instantiated in some Python config. I see the provided interface an extension of the PseudoInst interface, just that it didn't seem clean to embed the functionality in pseudo_inst.cc. The reason is that it makes usage simpler, as all that is needed is to invoke the pseudo insts. by some guest-workload (the guest workload decides the configuration, e.g. setting memory size, iterations, etc.). - Marco ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: http://reviews.gem5.org/r/3449/#review8290 ----------------------------------------------------------- On May 20, 2016, 11:45 a.m., Marco Elver wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > http://reviews.gem5.org/r/3449/ > ----------------------------------------------------------- > > (Updated May 20, 2016, 11:45 a.m.) > > > Review request for Default. > > > Repository: gem5 > > > Description > ------- > > Add support for McVerSi memory consistency verification framework > > This patch implements the Gem5-specific portion of McVerSi (a framework for > simulation-based memory consistency verification) [1]. > > Architectures supported are: > - ARM (current mc2lib code generation only supports ARMv7). > - X86 (pseudo ops available via mmapped IPR interface). > > Currently, only the O3CPU is supported. > > [1] http://ac.marcoelver.com/research/mcversi > > > Diffs > ----- > > src/arch/arm/isa/formats/m5ops.isa 54cf9a388a9d > src/cpu/o3/lsq_unit_impl.hh 54cf9a388a9d > src/cpu/o3/dyn_inst_impl.hh 54cf9a388a9d > src/cpu/o3/dyn_inst.hh 54cf9a388a9d > src/cpu/o3/commit_impl.hh 54cf9a388a9d > src/arch/arm/isa/insts/m5ops.isa 54cf9a388a9d > src/sim/pseudo_inst.hh 54cf9a388a9d > src/sim/mcversi.cc PRE-CREATION > src/sim/mcversi.hh PRE-CREATION > src/sim/SConscript 54cf9a388a9d > util/m5/m5ops.h 54cf9a388a9d > util/m5/m5op.h 54cf9a388a9d > util/m5/m5op_x86.S 54cf9a388a9d > src/sim/pseudo_inst.cc 54cf9a388a9d > src/arch/arm/isa/formats/aarch64.isa 54cf9a388a9d > > Diff: http://reviews.gem5.org/r/3449/diff/ > > > Testing > ------- > > Unless explicitly enabled (via loading appropriate workload), this component > is unused. > > Tested with ARM+Classic and X86+Ruby. Precompiled workloads that were used > for testing available here: > http://ac.marcoelver.com/res/mcversi_guest_workload_gem5.tar.gz > > However, bugs have been found elsewhere in Gem5 while testing McVerSi (see > http://www.mail-archive.com/[email protected]/msg18940.html , and 1 of 2 bugs > from paper http://reviews.gem5.org/r/2842/ ). (I will not restate them here > to keep the discussion on topic.) > > > Thanks, > > Marco Elver > > _______________________________________________ gem5-dev mailing list [email protected] http://m5sim.org/mailman/listinfo/gem5-dev
