> On 2011-06-20 14:18:52, Nathan Binkert wrote: > > src/arch/alpha/isa/mem.isa, line 279 > > <http://reviews.m5sim.org/r/750/diff/1/?file=13025#file13025line279> > > > > I have a couple of issues: > > 1) Where did the byte swapping come from? I don't see it elsewhere. > > > > 2) We've never used typeof before. Should we start? In the future > > (c++0x), we'll have auto, but typeof is nonstandard. > > > > Would it be bad to do the following? > > > > Mem = htog(Mem) > > and then just use Mem as the parameter to that function? Updating in > > place could of course be sketchy if the value is not ephemeral. > > > > If it is, could we not simply use: > > uint%(mem_acc_size)d_t gMem = htog(Mem); > > > > There are other options involving templates too. > > > > Gabe Black wrote: > 1) The byte swapping used to be done internal to the read/write functions > since they knew what the type was and how to swap it. The readBytes and > writeBytes functions don't, so that has to move up into the instructions. > > 2) I have no strong opinion about typeof. The reason I used that instead > of modifying Mem in place was that I didn't know for sure whether Mem would > be used after that as the code is now or in the future. I tried to update Mem > in place in templates where there wouldn't be anything after it. A major > motivation for this change is to make my previous change (which would be > applied after this one) that simplifies operand types able to deal with > signed and unsigned types in a (subjectively) less hacky way. I think those > changes get rid of the mem_acc_size field all together since the parser > wouldn't really know what the size is any more. I'd have to look at them > again to be sure. > > Nathan Binkert wrote: > 1) I saw it (I think in your other diff) > > 2) What is the type of Mem? I assume that the parser would have to know > what the type of Mem was, no? > > Steve Reinhardt wrote: > Can we keep the byte swapping in the read/write functions by passing in a > flag (like the ByteOrderDiffers flag in sim/byteswap.hh) that says whether > it's needed, then adding a switch on the data size to call the right > swap_byteNN function? Normally I might not suggest this, but it solves the > trace data problem, and if we're going to push the info in for that then we > might as well do all the byte swapping in one place. Also I expect that most > of the time the flag will be a compile-time constant false, so the > performance won't matter anyway.
Without a template, you have no type information, so you'd have to add a switch statement to get the correct swap. I still don't understand why the read/write functions in base_dyn_inst need to be removed. They handle everything correctly as far as I can tell. The base_dyn_inst knows its Impl and as a result should know its own endianness. I also don't quite understand this issue with signed/unsigned types. Is the type of Mem not necessarily the correct size? If so, then the byte swapping can't work. If it is, then I don't understand why we should be getting rid of the template versions of these functions which handle the byte swapping and tracing for us. - Nathan ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: http://reviews.m5sim.org/r/750/#review1343 ----------------------------------------------------------- On 2011-06-20 11:43:52, Gabe Black wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > http://reviews.m5sim.org/r/750/ > ----------------------------------------------------------- > > (Updated 2011-06-20 11:43:52) > > > Review request for Default, Ali Saidi, Gabe Black, Steve Reinhardt, and > Nathan Binkert. > > > Summary > ------- > > ISA: Use readBytes/writeBytes for all instruction level memory operations. > > > Diffs > ----- > > src/arch/alpha/isa/mem.isa f12d1cd32cc7 > src/arch/arm/isa/templates/mem.isa f12d1cd32cc7 > src/arch/mips/isa/formats/mem.isa f12d1cd32cc7 > src/arch/power/isa/formats/mem.isa f12d1cd32cc7 > src/arch/sparc/isa/formats/mem/swap.isa f12d1cd32cc7 > src/arch/sparc/isa/formats/mem/util.isa f12d1cd32cc7 > src/arch/x86/insts/microldstop.hh f12d1cd32cc7 > src/arch/x86/isa/microops/ldstop.isa f12d1cd32cc7 > > Diff: http://reviews.m5sim.org/r/750/diff > > > Testing > ------- > > > Thanks, > > Gabe > > _______________________________________________ gem5-dev mailing list [email protected] http://m5sim.org/mailman/listinfo/gem5-dev
