> On 2011-05-28 21:58:27, Steve Reinhardt wrote:
> > The description is a little general; can you be more specific about what 
> > this change is doing?  I see that you're getting rid of the size from the 
> > memory access operands and encoding that in the ctype instead, which seems 
> > fine.  However it seems like you've gotten rid of a lot of the signed vs. 
> > unsigned code, and made everything look unsigned, and I don't see how that 
> > still works.
> Gabe Black wrote:
>     The reason I changed them to unsigned is that all the read/write 
> functions have definitions generated for unsigned int operands but not signed 
> ones. They were being forced to unsigned when the call was being made. All of 
> the regressions passed with these changes and at the time I was convinced why 
> this worked, but it's been about a month since then and I don't really 
> remember the specifics. In SPARC and ARM, the Mem variables are being cast to 
> an appropriate type in the assignment, and in Alpha there were apparently no 
> signed Mem types. I'm guessing MIPS and Power don't have as extensive 
> regressions so problems may have slipped through due to those instructions 
> not being used or being used in a way that didn't expose a difference in 
> behavior. The simple fix is to add casts to those too in the few places where 
> it makes a difference, although there may be a reason that I'm not able to 
> remember that it works as is.
>     Generally speaking, signed vs. unsigned, size, and float vs. int 
> information only makes sense if you're building up a few preselected types as 
> the code is currently doing. By deferring more to the C++ type system, you 
> can use whatever type you want and it should just work.

Well, the fact that the old regressions didn't notice doesn't fill me with 
confidence, but at least it's more plausible that the updated version works.  I 
guess your old version may have worked in the MIPS case because the LHS is 
signed, e.g., lh is "Rt.sw = Mem.uw" while lhu is "Rt.uw = Mem.uw", but the LHS 
is not signed in the Power case, and even in MIPS the size on the LHS for lb is 
wrong (it's also Rt.sw and not Rt.sb), which may or may not matter.

Anyway, it seems very odd to have to say "(int8_t)Mem.ub" when we already have 
a ".sb" operand type defined as "int8_t".  It seems like a weird hidden 
restriction to say that there are operand types you can declare but can't use 
on memory, and that you're pushing a somewhat arbitrary internal implementation 
decision up to the level of language visibility, which is going the wrong 
direction.  I'm not sure what the right solution is, but even if it's the brute 
force one of creating a bunch more read/write function templates in the CPU 
implementations, I think that's preferable.

I must say that other than this one inconsistency, I like this change, and I'm 
left scratching my head about why I made the original version so complicated in 
the first place.

- Steve

This is an automatically generated e-mail. To reply, visit:

On 2011-05-30 00:18:31, Gabe Black wrote:
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://reviews.m5sim.org/r/655/
> -----------------------------------------------------------
> (Updated 2011-05-30 00:18:31)
> Review request for Default, Ali Saidi, Gabe Black, Steve Reinhardt, and 
> Nathan Binkert.
> Summary
> -------
> ISA parser: Simplify operand type handling.
> This change simplifies the code surrounding operand type handling and makes it
> depend only on the ctype that goes with each operand type. Future changes will
> allow defining operand types by their ctypes directly, convert the ISAs over
> to that style of definition, and then remove support for the old style. These
> changes are to make it easier to use non-builtin types like classes or
> structures as the type for operands.
> Diffs
> -----
>   src/arch/alpha/isa/mem.isa 03cfd2ecf6bb 
>   src/arch/arm/isa/insts/ldr.isa 03cfd2ecf6bb 
>   src/arch/arm/isa/insts/mem.isa 03cfd2ecf6bb 
>   src/arch/arm/isa/insts/str.isa 03cfd2ecf6bb 
>   src/arch/arm/isa/templates/mem.isa 03cfd2ecf6bb 
>   src/arch/isa_parser.py 03cfd2ecf6bb 
>   src/arch/mips/isa/decoder.isa 03cfd2ecf6bb 
>   src/arch/mips/isa/formats/mem.isa 03cfd2ecf6bb 
>   src/arch/power/isa/decoder.isa 03cfd2ecf6bb 
>   src/arch/power/isa/formats/mem.isa 03cfd2ecf6bb 
>   src/arch/sparc/isa/decoder.isa 03cfd2ecf6bb 
>   src/arch/sparc/isa/formats/mem/swap.isa 03cfd2ecf6bb 
>   src/arch/sparc/isa/formats/mem/util.isa 03cfd2ecf6bb 
> Diff: http://reviews.m5sim.org/r/655/diff
> Testing
> -------
> Thanks,
> Gabe

gem5-dev mailing list

Reply via email to