> 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.
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. > 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. 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. > 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. 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. > 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. 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. > 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. > > Ali Saidi wrote: > might as well be explicit It's not a huge thing, but if it's not required it's mostly visual noise. People will usually not care too much about exactly what type is being used, and if they do they can look at the function return type. - Gabe ----------------------------------------------------------- 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
