----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: http://reviews.gem5.org/r/3449/#review8290 -----------------------------------------------------------
src/arch/arm/isa/formats/aarch64.isa (line 2031) <http://reviews.gem5.org/r/3449/#comment7090> Please add comments to explain what these do. src/arch/arm/isa/formats/m5ops.isa (line 72) <http://reviews.gem5.org/r/3449/#comment7091> Same here src/arch/arm/isa/insts/m5ops.isa (line 569) <http://reviews.gem5.org/r/3449/#comment7093> As above, it would be good if these could be grouped with a comment around what they do and highighting that these all belong to McVersi functionality src/cpu/o3/commit_impl.hh (line 1269) <http://reviews.gem5.org/r/3449/#comment7094> Should this perhaps be a probe point? src/cpu/o3/dyn_inst.hh (line 104) <http://reviews.gem5.org/r/3449/#comment7095> Comments please src/cpu/o3/dyn_inst_impl.hh (line 180) <http://reviews.gem5.org/r/3449/#comment7092> Why this->? src/cpu/o3/dyn_inst_impl.hh (line 190) <http://reviews.gem5.org/r/3449/#comment7096> 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. src/cpu/o3/lsq_unit_impl.hh (line 131) <http://reviews.gem5.org/r/3449/#comment7097> again, should this be a probe point? src/sim/mcversi.hh (line 42) <http://reviews.gem5.org/r/3449/#comment7098> I don't think we do this anywhere else in the codebase. Just the author and title should be enough imho. src/sim/mcversi.hh (line 60) <http://reviews.gem5.org/r/3449/#comment7099> odd indentation, and no comments src/sim/mcversi.hh (line 66) <http://reviews.gem5.org/r/3449/#comment7110> void*? Please use C++11 STL data structures src/sim/mcversi.hh (line 70) <http://reviews.gem5.org/r/3449/#comment7109> naming convention src/sim/mcversi.cc (line 32) <http://reviews.gem5.org/r/3449/#comment7108> really? that seems dodgy src/sim/mcversi.cc (line 113) <http://reviews.gem5.org/r/3449/#comment7107> Could we make these part of a class instead of making them global? src/sim/mcversi.cc (line 116) <http://reviews.gem5.org/r/3449/#comment7100> Surely this should not be hardcoded here. Why not use the cache-line size from the System? src/sim/mcversi.cc (line 335) <http://reviews.gem5.org/r/3449/#comment7101> seems the isValid operations should be const src/sim/mcversi.cc (line 374) <http://reviews.gem5.org/r/3449/#comment7102> 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 src/sim/mcversi.cc (line 390) <http://reviews.gem5.org/r/3449/#comment7103> Should these not just be removed rather? src/sim/mcversi.cc (line 567) <http://reviews.gem5.org/r/3449/#comment7104> void*? Why not use vectors? src/sim/mcversi.cc (line 696) <http://reviews.gem5.org/r/3449/#comment7105> Please double check that this works with gcc 4.8 and clang 3.1 src/sim/pseudo_inst.cc (line 912) <http://reviews.gem5.org/r/3449/#comment7106> style - Andreas Hansson On May 5, 2016, 1:11 p.m., Marco Elver wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > http://reviews.gem5.org/r/3449/ > ----------------------------------------------------------- > > (Updated May 5, 2016, 1:11 p.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/aarch64.isa df24b9af42c7 > src/arch/arm/isa/formats/m5ops.isa df24b9af42c7 > src/arch/arm/isa/insts/m5ops.isa df24b9af42c7 > src/cpu/o3/commit_impl.hh df24b9af42c7 > src/cpu/o3/dyn_inst.hh df24b9af42c7 > src/cpu/o3/dyn_inst_impl.hh df24b9af42c7 > src/cpu/o3/lsq_unit_impl.hh df24b9af42c7 > src/sim/SConscript df24b9af42c7 > src/sim/mcversi.hh PRE-CREATION > src/sim/mcversi.cc PRE-CREATION > src/sim/pseudo_inst.hh df24b9af42c7 > src/sim/pseudo_inst.cc df24b9af42c7 > util/m5/m5op.h df24b9af42c7 > util/m5/m5op_x86.S df24b9af42c7 > util/m5/m5ops.h df24b9af42c7 > > 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
