----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: http://reviews.gem5.org/r/3269/#review7798 -----------------------------------------------------------
This makes good sense -- the new naming makes more clear the behavior of memory reads in timing mode. A couple minor nits below. src/cpu/exec_context.hh (lines 203 - 206) <http://reviews.gem5.org/r/3269/#comment6767> I presume the write path doesn't get the same treatment because it *does* need the same prototype? Why not repeat the initiate/write refactoring for symmetry and clarity? src/cpu/simple/atomic.cc (line 421) <http://reviews.gem5.org/r/3269/#comment6766> similar idea as comment on Timing src/cpu/simple/timing.cc (line 410) <http://reviews.gem5.org/r/3269/#comment6765> I would prefer that it proclaim something like "readMem() (an atomic memory interface) should never be called on TimingSimpleCPU!" - Curtis Dunham On Dec. 31, 2015, 8:10 a.m., Steve Reinhardt wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > http://reviews.gem5.org/r/3269/ > ----------------------------------------------------------- > > (Updated Dec. 31, 2015, 8:10 a.m.) > > > Review request for Default. > > > Repository: gem5 > > > Description > ------- > > Changeset 11287:e947130e058d > --------------------------- > cpu. arch: add initiateMemRead() to ExecContext interface > > For historical reasons, the ExecContext interface had a single > function, readMem(), that did two different things depending on > whether the ExecContext supported atomic memory mode (i.e., > AtomicSimpleCPU) or timing memory mode (all the other models). > In the former case, it actually performed a memory read; in the > latter case, it merely initiated a read access, and the read > completion did not happen until later when a response packet > arrived from the memory system. > > This led to some confusing things, including timing accesses > being required to provide a pointer for the return data even > though that pointer was only used in atomic mode. > > This patch splits this interface, adding a new initiateMemRead() > function to the ExecContext interface to replace the timing-mode > use of readMem(). > > For consistency and clarity, the readMemTiming() helper function > in the ISA definitions is renamed to initiateMemRead() as well. > For x86, where the access size is passed in explicitly, we can > also get rid of the data parameter at this level. For other ISAs, > where the access size is determined from the type of the data > parameter, we have to keep the parameter for that purpose. > > > Diffs > ----- > > src/arch/alpha/isa/mem.isa 4cc8b312f026321a81cb0b043e8e3a5c20b5f5db > src/arch/arm/isa/templates/mem.isa 4cc8b312f026321a81cb0b043e8e3a5c20b5f5db > src/arch/arm/isa/templates/mem64.isa > 4cc8b312f026321a81cb0b043e8e3a5c20b5f5db > src/arch/arm/isa/templates/neon64.isa > 4cc8b312f026321a81cb0b043e8e3a5c20b5f5db > src/arch/generic/memhelpers.hh 4cc8b312f026321a81cb0b043e8e3a5c20b5f5db > src/arch/mips/isa/formats/mem.isa 4cc8b312f026321a81cb0b043e8e3a5c20b5f5db > src/arch/power/isa/formats/mem.isa 4cc8b312f026321a81cb0b043e8e3a5c20b5f5db > src/arch/sparc/isa/formats/mem/util.isa > 4cc8b312f026321a81cb0b043e8e3a5c20b5f5db > src/arch/x86/isa/formats/monitor_mwait.isa > 4cc8b312f026321a81cb0b043e8e3a5c20b5f5db > src/arch/x86/isa/microops/ldstop.isa > 4cc8b312f026321a81cb0b043e8e3a5c20b5f5db > src/arch/x86/memhelpers.hh 4cc8b312f026321a81cb0b043e8e3a5c20b5f5db > src/cpu/base_dyn_inst.hh 4cc8b312f026321a81cb0b043e8e3a5c20b5f5db > src/cpu/exec_context.hh 4cc8b312f026321a81cb0b043e8e3a5c20b5f5db > src/cpu/minor/exec_context.hh 4cc8b312f026321a81cb0b043e8e3a5c20b5f5db > src/cpu/simple/atomic.hh 4cc8b312f026321a81cb0b043e8e3a5c20b5f5db > src/cpu/simple/atomic.cc 4cc8b312f026321a81cb0b043e8e3a5c20b5f5db > src/cpu/simple/base.hh 4cc8b312f026321a81cb0b043e8e3a5c20b5f5db > src/cpu/simple/exec_context.hh 4cc8b312f026321a81cb0b043e8e3a5c20b5f5db > src/cpu/simple/timing.hh 4cc8b312f026321a81cb0b043e8e3a5c20b5f5db > src/cpu/simple/timing.cc 4cc8b312f026321a81cb0b043e8e3a5c20b5f5db > > Diff: http://reviews.gem5.org/r/3269/diff/ > > > Testing > ------- > > > Thanks, > > Steve Reinhardt > > _______________________________________________ gem5-dev mailing list [email protected] http://m5sim.org/mailman/listinfo/gem5-dev
