> On 2011-03-29 10:02:01, Gabe Black wrote: > > 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.
Gabe, I think I agree with your comments. The intent with making the merge is to support some of the features necessary for ISAs like ARM and x86. But I had some reservations about keeping the translation and the unaligned memory access code in the BaseDynInst class, because in the InOrder model that stuff is handled separately in the "CacheUnit" resource for InOrder. It's done in a somewhat similar fashion to how the LSQ works in O3. However, there are issues say for split accesses whereas in the O3 model you try to make both requests on the same cycle (and fail if you don't), InOrder splits that up into separate requests to the cache allowing for overlap of the split request in high contention scenarios. The separate TLB translation is also done so that if the TLB is blocked/unavailable/etc. then you are not having to wait for 2 mshrs or 2 tlb-"bandwidth slots" to be available. With that said, I've been looking at the "CacheUnit" and "LSQ" implementations and now think that there is no reason that the DynInst can't make the request for a write and then the actual receiving object (LSQ or CacheUnit) can buffer the requests until the cache becomes available. The final "trick", so to speak, is for the receiving memory object to be able to tell that when all translations are done and also if the load/store was sent to memory successfully. I think the support I need to implement this is there though, so I'm going to update this patch with the generic translation and <read/write>Bytes support back in the Base class. If there are any problems with then getting that to work for InOrder, then I'll bring that up at that point. > On 2011-03-29 10:02:01, Gabe Black wrote: > > src/cpu/base_dyn_inst.hh, line 262 > > <http://reviews.m5sim.org/r/529/diff/1/?file=10611#file10611line262> > > > > This stuff looks old and I'm guessing should be deleted one way or the > > other. There's a slightly different way that InOrder handles this Result structure so I had planned to revisit this and merge it in after I merged inorder into this style of DynInst object. - Korey ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: http://reviews.m5sim.org/r/529/#review1029 ----------------------------------------------------------- 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
