> On May 20, 2012, 3:20 p.m., Steve Reinhardt wrote: > > In general I'm in agreement with this direction (as we've already > > discussed, but I just wanted to be clear about it). > > > > I'm curious why functions like extMachInstReady() and getExtMachInst() are > > still public... ideally I'd think that the ExtMachInst type(s) would now be > > entirely internal to the Decoder objects, and their definitions (not to > > mention their very existence) would be invisible to the CPU models. > > Gabe Black wrote: > That's a very reasonable thing to move towards, but outside of the scope > of what I'm trying to do. I started looking at it, and there are two problems > (which are not show stoppers) which make this less straightforward than it > could be. First, there's the noop ExtMachInst which is used, notably in O3, > to transport faults through the pipe. I think we definitely should have a > generic, ISA independent noop which can just be used without going through > the decoder, but because of the funny way the execute methods work it's not > possible, as far as I can see, to define an instruction object that can > actually be used outside of the ISA parser. I would like to change that > generally speaking by coming up with a C++ based mechanism which works > outside of generated code, but in the mean time that's fairly intractable. > Second, the idea of an ExtMachInst is plumbed all the way down into the base > StaticInst class. Extracting it and putting ExtMachInsts into the ISA > specific classes tha t actually need it shouldn't be very complicated at all, but it's still a decent amount of work and outside the scope of what I'm trying to do here.
Fair enough, I can believe it's not that simple to get rid of all traces of ExtMachInst outside the Decoder, even if that's the eventual goal. It still seems like extMachInstReady() should be renamed and getExtMachInst() made private though. Do we really care if the ExtMachInst is ready but not the StaticInst? I pointed out some specific examples in a separate review I just published. - Steve ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: http://reviews.gem5.org/r/1195/#review2754 ----------------------------------------------------------- On May 15, 2012, 5:56 a.m., Gabe Black wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > http://reviews.gem5.org/r/1195/ > ----------------------------------------------------------- > > (Updated May 15, 2012, 5:56 a.m.) > > > Review request for Default. > > > Description > ------- > > Changeset 9009:769d2142cbd4 > --------------------------- > CPU: Merge the predecoder and decoder. > > These classes are always used together, and merging them will give the ISAs > more flexibility in how they cache things and manage the process. > > > Diffs > ----- > > src/arch/SConscript f681719e2e99 > src/arch/alpha/decoder.hh PRE-CREATION > src/arch/alpha/isa/main.isa f681719e2e99 > src/arch/alpha/predecoder.hh f681719e2e99 > src/arch/arm/SConscript f681719e2e99 > src/arch/arm/decoder.hh PRE-CREATION > src/arch/arm/decoder.cc PRE-CREATION > src/arch/arm/predecoder.hh f681719e2e99 > src/arch/arm/predecoder.cc f681719e2e99 > src/arch/arm/types.hh f681719e2e99 > src/arch/mips/decoder.hh PRE-CREATION > src/arch/mips/predecoder.hh f681719e2e99 > src/arch/power/decoder.hh PRE-CREATION > src/arch/sparc/decoder.hh PRE-CREATION > src/arch/sparc/predecoder.hh f681719e2e99 > src/arch/x86/SConscript f681719e2e99 > src/arch/x86/decoder.hh PRE-CREATION > src/arch/x86/decoder.cc PRE-CREATION > src/arch/x86/decoder_tables.cc PRE-CREATION > src/arch/x86/emulenv.cc f681719e2e99 > src/arch/x86/isa/decoder/one_byte_opcodes.isa f681719e2e99 > src/arch/x86/predecoder.hh f681719e2e99 > src/arch/x86/predecoder.cc f681719e2e99 > src/arch/x86/predecoder_tables.cc f681719e2e99 > src/arch/x86/types.hh f681719e2e99 > src/cpu/base.hh f681719e2e99 > src/cpu/checker/cpu.hh f681719e2e99 > src/cpu/checker/cpu_impl.hh f681719e2e99 > src/cpu/inorder/cpu.hh f681719e2e99 > src/cpu/inorder/cpu.cc f681719e2e99 > src/cpu/inorder/resources/cache_unit.hh f681719e2e99 > src/cpu/inorder/resources/cache_unit.cc f681719e2e99 > src/cpu/inorder/resources/fetch_unit.hh f681719e2e99 > src/cpu/inorder/resources/fetch_unit.cc f681719e2e99 > src/cpu/inorder/thread_context.hh f681719e2e99 > src/cpu/legiontrace.cc f681719e2e99 > src/cpu/o3/fetch.hh f681719e2e99 > src/cpu/o3/fetch_impl.hh f681719e2e99 > src/cpu/simple/atomic.cc f681719e2e99 > src/cpu/simple/base.hh f681719e2e99 > src/cpu/simple/base.cc f681719e2e99 > src/cpu/simple_thread.cc f681719e2e99 > > Diff: http://reviews.gem5.org/r/1195/diff/ > > > Testing > ------- > > > Thanks, > > Gabe Black > > _______________________________________________ gem5-dev mailing list [email protected] http://m5sim.org/mailman/listinfo/gem5-dev
