On 05/30/11 09:47, Steve Reinhardt wrote: > >> 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
I'm replying in email instead of on review board since this is more of a general discussion than review feedback. The unsigned thing is sort of a weird gotcha. I'd like to avoid adding a lot of bulk to the CPU models since they're already fairly chunky and it makes them harder to reason about looking through the code, but it would be great to straighten this out. One possibility might be the technique I used with the endianness changing functions in src/sim/byteswap.hh. Things are built up in layers there: The swapping functions that depend on the guest byte order are templated on the type being swapped and are defined in terms of the functions that use a fixed endianness on the guest end. The functions that depend on the host and not the guest are templated on the type as well, and use the preprocessor to pick versions that do swapping or not based on the endianness of the host. They're defined using the swap_byte function which is also templated on the type. The functions that depend on neither the guest nor the host are defined unconditionally, templated on the type, and use swap_byte for their implementation. The swap_byte function is templated on the type it's swapping. It's implemented as an if that selects a swapping function based on the size of the type involved. Because it's inlined and templated on a particular type, the compiler should squash the if (and the function entirely) so that the right thing is being called. Then, finally, the swap_byteXX functions are called where XX is the type's size in bits. So with all these layers, the compiler can select the appropriate byte swapping operation (or none at all) based on the starting and ending orders, the host and guest orders, and the size of the type without having to actually know about the type ahead of time, assuming it's 1, 2, 4, or 8 bytes in size. We might be able to do something like this for the execution context functions so that they can read anything as long as it's a recognized size. That could even get rid of some redundancy in those implementations. A major catch might be if these have to be virtual functions at some point, since the compiler won't be as good at collapsing those and there are weird interactions between that and templating, I think. Gabe _______________________________________________ gem5-dev mailing list gem5-dev@m5sim.org http://m5sim.org/mailman/listinfo/gem5-dev
