----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: http://reviews.m5sim.org/r/529/#review1029 -----------------------------------------------------------
I agree with the sentiment of this change, but I think you moved too much into the O3 class. There's functionality (pointed out below) that you'll need to support in InOrder to be compliant with the interface instructions expect from CPUs, specifically delayed translation and oddly shaped/sized, memory accesses with readBytes/writeBytes. You'll have to support those to run all the ISAs, as would any other CPU using a dyninst in the future. The implementations in the base dyninst are pretty generic, although feel free to point out why they might not work with InOrder. src/cpu/base_dyn_inst.hh <http://reviews.m5sim.org/r/529/#comment1397> You're going to need to support readBytes and writeBytes style loads and stores to run all the ISAs and to conform to the interfaces the CPU is supposed to provide. These implementations seem pretty generic to me and should work with InOrder, although I don't actually know that much about InOrder. src/cpu/base_dyn_inst.hh <http://reviews.m5sim.org/r/529/#comment1398> You're also going to need to support delayed translation for X86 or ARM. It's important we don't have CPUs diverge in what functionality they provide. src/cpu/base_dyn_inst.hh <http://reviews.m5sim.org/r/529/#comment1399> This stuff looks old and I'm guessing should be deleted one way or the other. src/cpu/base_dyn_inst.hh <http://reviews.m5sim.org/r/529/#comment1400> This seems pretty generic and necessary for delayed translations. I think you probably need to update InOrder to support it rather than isolating it to O3. src/cpu/base_dyn_inst.hh <http://reviews.m5sim.org/r/529/#comment1401> Ditto src/cpu/o3/dyn_inst.hh <http://reviews.m5sim.org/r/529/#comment1402> As mentioned above, this "result" stuff may just be old cruft. You should check to see if it's used at all, and if not just let it go away. - Gabe On 2011-03-01 13:49:24, Korey Sewell wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > http://reviews.m5sim.org/r/529/ > ----------------------------------------------------------- > > (Updated 2011-03-01 13:49:24) > > > Review request for Default, Ali Saidi, Gabe Black, Steve Reinhardt, and > Nathan Binkert. > > > Summary > ------- > > cpu: split o3-specific parts out of BaseDynInst > The bigger picture goal is that I want to get the InorderDynInst class > derived from the > BaseDynInst, since there is no need to replicate a lot of useful code already > defined > in BaseDynInst (e.g. microcode identification, etc.) and Inorder can take > advantage > of common code that handles microcode and other features that other ISAs need. > > But to do this, there are a lot of o3-specific things that are in > BaseDynInst, that I pushed to > O3DynInst in this patch. Book-keeping variables that handle the IQ,LSQ,ROB > are unnecessary in > the base class but generic variables that will work across CPUs (IsSquashed, > IsCompleted, etc.) > are kept in the base class. > > The upside is more consistency across the simple models (branch prediction > and instruction > identification are all in one common place). > > I really wanted to define pure virtual functions for read/write(to memory) > and the > set<Int/Float>RegOperand, but virtual functions in a templated class is a > no-no and > I couldn't get around that (suggestions?). > > Also, I'd rather not use the "this->" pointer all over the place to access > member variables of > the templated Base class, but it had to be done. > > Other than those quirks, simulator functionality should stay the same as the > O3 Model always references > the O3DynInst pointer and the InOrder model doesnt currently make use of the > base dyn inst. class. > (but it will be easier to derive from now...) > > > Diffs > ----- > > src/cpu/base_dyn_inst.hh cf1afc88070f > src/cpu/base_dyn_inst_impl.hh cf1afc88070f > src/cpu/o3/dyn_inst.hh cf1afc88070f > src/cpu/o3/dyn_inst_impl.hh cf1afc88070f > > Diff: http://reviews.m5sim.org/r/529/diff > > > Testing > ------- > > > Thanks, > > Korey > > _______________________________________________ m5-dev mailing list m5-dev@m5sim.org http://m5sim.org/mailman/listinfo/m5-dev
