> 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? > > Ali Saidi wrote: > 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. > > Gabe Black wrote: > The reason I implemented sending diffs of the state instead of sending > the whole state is that what you send over the wire -does- matter, > significantly. Breakpoints are slow too, but that doesn't mean everything > else is irrelevant.
I fully appreciate that it wouldn't be a good idea to send 1KB of data over the wire, but we're well past bike shedding arguing about 4 bytes. There is no reason to add complexity and control flow to try and avoid it. > 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]). > > Ali Saidi wrote: > 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. > > Gabe Black wrote: > uregs changing size is irrelevant. That formula will be exactly right all > the time and doesn't depend on the coincidence that the CPSR is last. Only if you assume that the integer registers are sent first, which it seems like you're unwilling to make any assumptions about the code, so perhaps we should somehow verify that? > 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. > > Ali Saidi wrote: > 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. > > Gabe Black wrote: > I wrote the original assert and know what it's for, verifying the index > and not the structure. Again, it assumes F0 is the first FP reg which is > arbitrary. No it's not! F0 Is the first floating point register. 0 is the first whole number so it's first. Would you rather some uglyiness of START_FP, F0 = START_FP? Why are we arguing about code that is correct by inspection, has been extensively tested, and works?! > 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. > > Ali Saidi wrote: > I disagree... This will transition nicely as soon as libc gets it's act > together. > > Gabe Black wrote: > The fact that gcc uses macros is an unfortunate historical artifact, not > a valid design decision. The transition won't be that nice either since it'll > leave macro cruft in the source. We should either use their macro because we > have to, or use our own thing that doesn't suck because it makes sense. Here > we're combining the downsides of both of those approaches. Sure, but if for some reason the value changes when the support actually makes it into libc it will be correct. - 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
