> On May 20, 2012, 5:44 p.m., Steve Reinhardt wrote: > > > > Gabe Black wrote: > I meant to also say that I'm not trying to change the interface to the > decoder/predecoder combo here. That would come later and is outside the scope > of this change.
OK, I guess I'm just confused because there are places where you did change the interface more significantly, like the change in cpu/simple/base.cc around lines 380-400, getting rid of extMachInstReady() and getExtMachInst() entirely, and I was hoping to see that kind of transformation everywhere that those functions were used. > On May 20, 2012, 5:44 p.m., Steve Reinhardt wrote: > > src/cpu/legiontrace.cc, line 430 > > <http://reviews.gem5.org/r/1195/diff/1/?file=26411#file26411line430> > > > > the concept of "extMachInst" is meaningless now, so this function name > > doesn't make sense in context. should it be decoderReady() or something > > like that? > > Gabe Black wrote: > It isn't meaningless yet. Also, the decoder being ready is ambiguous. > Ready for what? More bytes? To return an instruction? Well what does extMachInstReady() mean when we're not using it to decide whether to call getExtMachInst() anymore? > On May 20, 2012, 5:44 p.m., Steve Reinhardt wrote: > > src/cpu/o3/fetch_impl.hh, line 1198 > > <http://reviews.gem5.org/r/1195/diff/1/?file=26413#file26413line1198> > > > > similar thing here... how is !extMachInstReady() at this point > > different from needMoreBytes()? The latter sounds more intuitively like > > what you want to call based on the name of the flag. > > Gabe Black wrote: > !extMachInstReady is not the same as needMoreBytes at all. There may be > many instructions in the same MachInst blob of bytes. There may be many > MachInst blobs before we get a complete instruction. I figured that they weren't the same, but I don't understand the difference. Intuitively I'd think that needMoreBytes() means "need more bytes to make a complete instruction". I don't see any comments that explain the difference. Anyway, I'm not sure where this interface is headed, but given some of the changes you made, it seems like some additional changes like renaming extMachInstReady() to have a name that's meaningful even when you're not calling getExtMachInst() (assuming it's not about to go away entirely), and making uniform changes to places where instructions get decoded (i.e., taking whatever you did in simple/base.cc and doing it to the checker too), would help to make this change overall more consistent. - Steve ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: http://reviews.gem5.org/r/1195/#review2763 ----------------------------------------------------------- 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
