Re: [m5-dev] Review Request: cpu: split o3-specific parts out of BaseDynInst
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/writeBytes 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 setInt/FloatRegOperand, 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
Re: [m5-dev] Review Request: cpu: split o3-specific parts out of BaseDynInst
--- 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 setInt/FloatRegOperand, 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
Re: [m5-dev] Review Request: cpu: split o3-specific parts out of BaseDynInst
On 2011-03-03 20:41:09, Ali Saidi wrote: Please don't ship this until I have a chance to try it, I just want to make sure it doesn't break ARM_FS/O3. Korey Sewell wrote: Sure, I'd welcome a go of things from some other folks to test if I haven't introduced something quirky. After there is some commentary, I'll make sure to run the full regressions before committing this because as we all know BaseDynInst is a pretty fundamental part of M5. Also, I'll be posting an update to this diff soon that will make the setInt/FloatRegOperand pure virtual (I mistakenly thought those were templated member functions in the first go-round). Ali Saidi wrote: Anything happen with your update diff? If you could verify it passes the arm/o3 full system regression I just committed and then I'll give it a go on a bunch more tests. Korey Sewell wrote: Gabe made a good point about the virtual function overhead on the commonly used set*Operand functions and I've just been waffling on whether to even make those pure virtual or not. However, I'll go ahead and take a hard look at this again , run the regressions, and post an update tomorrow so we can move on with this potential change. Korey Sewell wrote: The ARM regressions pass with this patch. Feel free to do further testing. Ali Saidi wrote: What about the virtual function overhead? Korey Sewell wrote: This patch I posted didn't have any virtual functions in it. I was speculating whether or not we should add the pure virtual functions to solidify the DynInst interface. I went ahead and added the virtual functions in a separate patch, ran the ARM_FS o3 regressions 2x on the same machine, and then took the inst/second from the generated m5 stat files: o3-timing (current): 115133, 115406 o3-timing (this patch): 115011, 115709 o3-timing (this patch w/pure virtual functions on BaseDynInst read/set*Operands): 114368,113055 Although running something 2x isn't anything that could be called thorough, it looks like adding the virtual functions incurs a overhead of about 1K-2K inst/second, while the patch as currently constructed gives about the same performance as the code that doesn't split the o3-specific code out of BaseDynInst. Since few people actually work on the internals of the CPU models anyway, it's probably better to go with the patch submitted (pending further ARM-FS testing) than to sacrifice the small amount of performance for an interface that would rarely, if ever, be extended past the Inorder/O3 models. Can I assume that this patch is OK to push (assuming a final run of all the o3-regression tests pass???) - Korey --- This is an automatically generated e-mail. To reply, visit: http://reviews.m5sim.org/r/529/#review932 --- 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 setInt/FloatRegOperand, 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
Re: [m5-dev] Review Request: cpu: split o3-specific parts out of BaseDynInst
On 2011-03-03 20:41:09, Ali Saidi wrote: Please don't ship this until I have a chance to try it, I just want to make sure it doesn't break ARM_FS/O3. Korey Sewell wrote: Sure, I'd welcome a go of things from some other folks to test if I haven't introduced something quirky. After there is some commentary, I'll make sure to run the full regressions before committing this because as we all know BaseDynInst is a pretty fundamental part of M5. Also, I'll be posting an update to this diff soon that will make the setInt/FloatRegOperand pure virtual (I mistakenly thought those were templated member functions in the first go-round). Ali Saidi wrote: Anything happen with your update diff? If you could verify it passes the arm/o3 full system regression I just committed and then I'll give it a go on a bunch more tests. Korey Sewell wrote: Gabe made a good point about the virtual function overhead on the commonly used set*Operand functions and I've just been waffling on whether to even make those pure virtual or not. However, I'll go ahead and take a hard look at this again , run the regressions, and post an update tomorrow so we can move on with this potential change. Korey Sewell wrote: The ARM regressions pass with this patch. Feel free to do further testing. Ali Saidi wrote: What about the virtual function overhead? This patch I posted didn't have any virtual functions in it. I was speculating whether or not we should add the pure virtual functions to solidify the DynInst interface. I went ahead and added the virtual functions in a separate patch, ran the ARM_FS o3 regressions 2x on the same machine, and then took the inst/second from the generated m5 stat files: o3-timing (current): 115133, 115406 o3-timing (this patch): 115011, 115709 o3-timing (this patch w/pure virtual functions on BaseDynInst read/set*Operands): 114368,113055 Although running something 2x isn't anything that could be called thorough, it looks like adding the virtual functions incurs a overhead of about 1K-2K inst/second, while the patch as currently constructed gives about the same performance as the code that doesn't split the o3-specific code out of BaseDynInst. Since few people actually work on the internals of the CPU models anyway, it's probably better to go with the patch submitted (pending further ARM-FS testing) than to sacrifice the small amount of performance for an interface that would rarely, if ever, be extended past the Inorder/O3 models. - Korey --- This is an automatically generated e-mail. To reply, visit: http://reviews.m5sim.org/r/529/#review932 --- 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 setInt/FloatRegOperand, 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
Re: [m5-dev] Review Request: cpu: split o3-specific parts out of BaseDynInst
On 2011-03-03 20:41:09, Ali Saidi wrote: Please don't ship this until I have a chance to try it, I just want to make sure it doesn't break ARM_FS/O3. Korey Sewell wrote: Sure, I'd welcome a go of things from some other folks to test if I haven't introduced something quirky. After there is some commentary, I'll make sure to run the full regressions before committing this because as we all know BaseDynInst is a pretty fundamental part of M5. Also, I'll be posting an update to this diff soon that will make the setInt/FloatRegOperand pure virtual (I mistakenly thought those were templated member functions in the first go-round). Ali Saidi wrote: Anything happen with your update diff? If you could verify it passes the arm/o3 full system regression I just committed and then I'll give it a go on a bunch more tests. Korey Sewell wrote: Gabe made a good point about the virtual function overhead on the commonly used set*Operand functions and I've just been waffling on whether to even make those pure virtual or not. However, I'll go ahead and take a hard look at this again , run the regressions, and post an update tomorrow so we can move on with this potential change. The ARM regressions pass with this patch. Feel free to do further testing. - Korey --- This is an automatically generated e-mail. To reply, visit: http://reviews.m5sim.org/r/529/#review932 --- 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 setInt/FloatRegOperand, 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
Re: [m5-dev] Review Request: cpu: split o3-specific parts out of BaseDynInst
On 2011-03-03 20:41:09, Ali Saidi wrote: Please don't ship this until I have a chance to try it, I just want to make sure it doesn't break ARM_FS/O3. Korey Sewell wrote: Sure, I'd welcome a go of things from some other folks to test if I haven't introduced something quirky. After there is some commentary, I'll make sure to run the full regressions before committing this because as we all know BaseDynInst is a pretty fundamental part of M5. Also, I'll be posting an update to this diff soon that will make the setInt/FloatRegOperand pure virtual (I mistakenly thought those were templated member functions in the first go-round). Anything happen with your update diff? If you could verify it passes the arm/o3 full system regression I just committed and then I'll give it a go on a bunch more tests. - Ali --- This is an automatically generated e-mail. To reply, visit: http://reviews.m5sim.org/r/529/#review932 --- 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 setInt/FloatRegOperand, 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
Re: [m5-dev] Review Request: cpu: split o3-specific parts out of BaseDynInst
On 2011-03-03 20:41:09, Ali Saidi wrote: Please don't ship this until I have a chance to try it, I just want to make sure it doesn't break ARM_FS/O3. Korey Sewell wrote: Sure, I'd welcome a go of things from some other folks to test if I haven't introduced something quirky. After there is some commentary, I'll make sure to run the full regressions before committing this because as we all know BaseDynInst is a pretty fundamental part of M5. Also, I'll be posting an update to this diff soon that will make the setInt/FloatRegOperand pure virtual (I mistakenly thought those were templated member functions in the first go-round). Ali Saidi wrote: Anything happen with your update diff? If you could verify it passes the arm/o3 full system regression I just committed and then I'll give it a go on a bunch more tests. Gabe made a good point about the virtual function overhead on the commonly used set*Operand functions and I've just been waffling on whether to even make those pure virtual or not. However, I'll go ahead and take a hard look at this again , run the regressions, and post an update tomorrow so we can move on with this potential change. - Korey --- This is an automatically generated e-mail. To reply, visit: http://reviews.m5sim.org/r/529/#review932 --- 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 setInt/FloatRegOperand, 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
Re: [m5-dev] Review Request: cpu: split o3-specific parts out of BaseDynInst
--- This is an automatically generated e-mail. To reply, visit: http://reviews.m5sim.org/r/529/#review932 --- Please don't ship this until I have a chance to try it, I just want to make sure it doesn't break ARM_FS/O3. - Ali 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 setInt/FloatRegOperand, 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
Re: [m5-dev] Review Request: cpu: split o3-specific parts out of BaseDynInst
On 2011-03-03 20:41:09, Ali Saidi wrote: Please don't ship this until I have a chance to try it, I just want to make sure it doesn't break ARM_FS/O3. Sure, I'd welcome a go of things from some other folks to test if I haven't introduced something quirky. After there is some commentary, I'll make sure to run the full regressions before committing this because as we all know BaseDynInst is a pretty fundamental part of M5. Also, I'll be posting an update to this diff soon that will make the setInt/FloatRegOperand pure virtual (I mistakenly thought those were templated member functions in the first go-round). - Korey --- This is an automatically generated e-mail. To reply, visit: http://reviews.m5sim.org/r/529/#review932 --- 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 setInt/FloatRegOperand, 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
Re: [m5-dev] Review Request: cpu: split o3-specific parts out of BaseDynInst
I think there's been a lot of aversion to virtual functions in the past because they add some performance overhead. You've got the indirection through what's basically a function pointer, and you can't really inline the function because you don't know what it's going to be ahead of time. The set*Operand functions are pretty frequently called, too. For more complex CPUs like O3 and InOrder it might all get lost in the noise, but please at least measure what the impact is performance wise. Gabe On 03/03/11 21:06, Korey Sewell wrote: On 2011-03-03 20:41:09, Ali Saidi wrote: Please don't ship this until I have a chance to try it, I just want to make sure it doesn't break ARM_FS/O3. Sure, I'd welcome a go of things from some other folks to test if I haven't introduced something quirky. After there is some commentary, I'll make sure to run the full regressions before committing this because as we all know BaseDynInst is a pretty fundamental part of M5. Also, I'll be posting an update to this diff soon that will make the setInt/FloatRegOperand pure virtual (I mistakenly thought those were templated member functions in the first go-round). - Korey --- This is an automatically generated e-mail. To reply, visit: http://reviews.m5sim.org/r/529/#review932 --- 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 setInt/FloatRegOperand, 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 ___ m5-dev mailing list m5-dev@m5sim.org http://m5sim.org/mailman/listinfo/m5-dev
[m5-dev] Review Request: cpu: split o3-specific parts out of BaseDynInst
--- This is an automatically generated e-mail. To reply, visit: http://reviews.m5sim.org/r/529/ --- 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 setInt/FloatRegOperand, 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
