I have some CLs up for review which should implement what I described here, although they aren't extensively tested. Please give them a look if you have a chance. I combined the new patch series with the existing register type one since they all go together and interact with each other, so those are rebased and interposed in the middle of the new ones as early as possible while letting the new patches fix the problem Giacomo found before it's introduced.
On Mon, Nov 19, 2018 at 3:47 PM Gabe Black <[email protected]> wrote: > Hi folks. Over on my review here: > > https://gem5-review.googlesource.com/c/public/gem5/+/13622 > > Giacomo kindly diagnosed a bug in my CL which has to do with the FloatReg > and FloatRegBits types and the accessors which get and set those values. > > In a nut shell, I wanted to make FloatReg universally double, and > FloatRegBits universally uint64_t. That would mean that all the places that > use those types (generic register accessors, for instance) wouldn't need to > know what ISA was in use, since the types would be invariant. My idea was > that this would work out because if you wanted a uint32_t but got a > uint64_t, you'd just truncate it, and if you wanted a float but got a > double the compiler would trivially down convert it. The same sort of > conversions would happen in the other direction as well. > > The problem arises from how the (read|set)FloatReg and > (read|set)FloatRegBits functions interact with each other. > > Lets say you want to store a 32 bit floating point value in a register, > and then later read that out as a 32 bit bit pattern which represents that > value, maybe to interpret it as an int, or for whatever other purpose. > > If you had floats and uint32_ts plumbed all the way through, you would > store that value as a float, and then get it's binary uint32_t > representation in a very straightforward way. > > float -> uin32_t > > With my new scheme however, there's an extra step in the middle which > doesn't fold away properly and gets you a different answer. > > float -> double -> uint64_t -> uint32_t > > The float to double conversion is the result of upconverting for the > setFloatReg function's argument. Then the "conversion" from double to > uin64_t is done by having both in a union and getting the uint64_t member > out. The final conversion from uint64_t to uint32_t is a truncation. > > So in this version, what you get is a 32 bit truncated version of the > binary representation of the floating point value as a double. Or in > simpler words, the double chopped in half instead of the float. > > > I've thought about this a little bit, and, apart from giving up on the > whole thing which I'd rather not do, the nicest solution that I've thought > of would be to remove the accessors which work with floating point values > and only ever work with integer bitwise representations of those registers. > Also I would add some small, inline utility conversion functions to > translate between the bitwise and floating point representations at the > different widths. > > If you wanted to read a floating point value as a float, you would > therefore read it as a uint64_t, and pass it through the converter to get a > float of the size you wanted. If you want to store a float, you would first > convert it to whatever sized uintxx_t was appropriate (float -> uint32_t, > double -> uint64_t) and then store that. > > This does have the downside that extracting floating point values from the > floating point register file (what it's for, right?) becomes a bit more > cumbersome, but I think that's mitigated almost entirely by the fact that > those calls are almost all/all generated by the ISA parser. > > On the plus side, it should make what I was originally trying to do work, > and it will also remove a raft of plumbing which pipes those values through > various interfaces > > > I'm going to start working on this and put up some CLs at some point. > Feedback welcome. > > Gabe > _______________________________________________ gem5-dev mailing list [email protected] http://m5sim.org/mailman/listinfo/gem5-dev
