> On 2011-05-02 16:42:25, Gabe Black wrote: > > util/statetrace/arch/arm/tracechild.cc, line 151 > > <http://reviews.m5sim.org/r/669/diff/1/?file=12215#file12215line151> > > > > Is the cast actually necessary here? I can believe it is to avoid a > > warning, but you could try leaving it out if you haven't already.
might as well be explicit > On 2011-05-02 16:42:25, Gabe Black wrote: > > util/statetrace/arch/arm/tracechild.cc, line 129 > > <http://reviews.m5sim.org/r/669/diff/1/?file=12215#file12215line129> > > > > Just because libc would use a macro doesn't mean we have to. You should > > replace this with a constant of the appropriate type. I disagree... This will transition nicely as soon as libc gets it's act together. > On 2011-05-02 16:42:25, Gabe Black wrote: > > util/statetrace/arch/arm/tracechild.cc, line 79 > > <http://reviews.m5sim.org/r/669/diff/1/?file=12215#file12215line79> > > > > I don't know how easy this would be to accommodate, but you're going to > > be sending a bunch of extra zeros for int regs that aren't 64 bits wide. > > Can you make it so you send full 64 bit values only when the source is > > actually 64 bits wide? There is no reason to worry about sending 4 bytes down the wire. The speed issue isn't about sending 4 bytes, it's all about having to put a breakpoint after every instruction. > On 2011-05-02 16:42:25, Gabe Black wrote: > > util/statetrace/arch/arm/tracechild.cc, line 104 > > <http://reviews.m5sim.org/r/669/diff/1/?file=12215#file12215line104> > > > > The idea is to verify that you're not falling off of uregs. Maybe you > > could do something more flexible like sizeof(myregs.uregs) / sizeof > > (myregs.uregs[0]). uregs is never going to get smaller than it is now and I don't see a reason to come up with a crazy assert to try and prove that it isn't. > On 2011-05-02 16:42:25, Gabe Black wrote: > > util/statetrace/arch/arm/tracechild.cc, line 111 > > <http://reviews.m5sim.org/r/669/diff/1/?file=12215#file12215line111> > > > > The same comment applies as in getRegs, except that you have to deal > > with an offset. It would probably be a good idea to define something in the > > enum to mark the start of the FP regs. You can move the assert to after the > > if and subtract out the offset right before indexing fpregs. There are clearly 32 float registers defined in the enum and in the struct. The assert just verifies that we're actually accessing a floating point register when we should be. We don't need to verify the structure size it's correct by construction. > On 2011-05-02 16:42:25, Gabe Black wrote: > > src/arch/arm/nativetrace.cc, line 123 > > <http://reviews.m5sim.org/r/669/diff/1/?file=12213#file12213line123> > > > > You should divide by two here instead of shifting by 1. It's more > > obvious what you're doing, and the compiler will be smart enough to use a > > shift it it's faster. Personally, I like the look of the shift better, although I'm sure that the compiler can figure it out. > On 2011-05-02 16:42:25, Gabe Black wrote: > > src/arch/arm/nativetrace.cc, line 124 > > <http://reviews.m5sim.org/r/669/diff/1/?file=12213#file12213line124> > > > > spaces around the + done - Ali ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: http://reviews.m5sim.org/r/669/#review1181 ----------------------------------------------------------- On 2011-05-02 15:41:28, Ali Saidi wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > http://reviews.m5sim.org/r/669/ > ----------------------------------------------------------- > > (Updated 2011-05-02 15:41:28) > > > Review request for Default, Ali Saidi, Gabe Black, Steve Reinhardt, and > Nathan Binkert. > > > Summary > ------- > > ARM: Add vfpv3 support to native trace. > > > Diffs > ----- > > src/arch/arm/nativetrace.hh 3f49ed206f46 > src/arch/arm/nativetrace.cc 3f49ed206f46 > util/statetrace/arch/arm/tracechild.hh 3f49ed206f46 > util/statetrace/arch/arm/tracechild.cc 3f49ed206f46 > > Diff: http://reviews.m5sim.org/r/669/diff > > > Testing > ------- > > > Thanks, > > Ali > > _______________________________________________ m5-dev mailing list m5-dev@m5sim.org http://m5sim.org/mailman/listinfo/m5-dev
