[PATCH] D46915: [Fixed Point Arithmetic] Fixed Point Precision Bits and Fixed Point Literals

[Bevin Hansson via Phabricator via cfe-commits]

ebevhan added inline comments.


================
Comment at: include/clang/Basic/TargetInfo.h:89
+  // corresponding unsaturated types.
+  unsigned char ShortAccumFBits, ShortAccumIBits;
+  unsigned char AccumFBits, AccumIBits;
----------------
leonardchan wrote:
> ebevhan wrote:
> > leonardchan wrote:
> > > ebevhan wrote:
> > > > I suspect it's still possible to calculate the ibits based on the fbits, even for _Accum.
> > > > 
> > > > Are the unsigned values needed? The fbits for unsigned _Fract are the same as for signed _Fract if SameFBits is set, and +1 otherwise. The same should go for unsigned _Accum, but I don't think it's entirely clear how this affects the integral part.
> > > Similar to the previous comment, if we choose to make SameFBits dependent on/controlled by the target, then I think it would be better for the target to explicitly specify the integral bits since there could be some cases where there may be padding (such as for unsigned _Accum types if this flag is set), and in this case, I think it should be explicit that the `bit_width != IBits + FBits`.
> > > 
> > > We also can't fill in that padding for the unsigned _Accum types as an extra integral bit since the standard says that `"each signed accum type has at least as many integral bits as its corresponding unsigned accum type".`
> > > 
> > > For the unsigned _Fract values, I think we can get rid of them if we choose to keep the flag instead, but it would no longer apply to unsigned _Accum types since it would allow for extra padding in these types and would conflict with the logic of `bit_width == IBits + FBits`
> > > 
> > > For now, I actually think the simplest option is to keep these target properties, but have the target override them individually, with checks to make sure the values adhere to the standard.
> > Is it not the case that `bitwidth != IBits + FBits` for signed types only? Signed types require a sign bit (which is neither a fractional bit nor an integral bit, according to spec) but unsigned types do not, and if IBits and FBits for the signed and unsigned types are the same, the MSB is simply a padding bit, at least for _Accum (for _Fract everything above the fbits is padding). 
> > 
> > My reasoning for keeping the number of configurable values down was to limit the number of twiddly knobs to make the implementation simpler. Granting a lot of freedom is nice, but I suspect that it will be quite hard to get the implementation working properly for every valid configuration. I also don't really see much of a reason for `FBits != UFBits` in general. I know the spec gives a recommendation to implement it that way, but I think the benefit of normalizing the signed and unsigned representations outweighs the lost bit in the unsigned type.
> > 
> > It's hard to say what the differences are beyond that since I'm not sure how you plan on treating padding bits. In our implementation, padding bits (the MSB in all of the unsigned types for us) after any operation are zeroed.
> I see. The implementation would be much simpler this way. The initial idea was to treat the padding bits as "don't care" bits where we would mask only the data bits when doing operations like comparisons that look at the whole integer.
That does work, but for binary operations it means you might need multiple maskings per operation. If you mask after every operation instead, you only need one. In our implementation, the only padding bit we have is the MSB in the unsigned types. We simply insert a 'bit clear' after every operation that produces an unsigned type (with some exceptions).

The E-C spec says that padding bits are 'unspecified', but implementation-wise, they have to be defined to something in order to operate on them. So long as you ensure that it is never possible to observe anything but zero in those bits from a program perspective, most operations just work. Naturally, there are ways to subvert this design. If you have a `signed _Fract *` and cast it to an `unsigned _Fract *` and the values happen to be negative, the padding bit will be set and you'll get exciting behavior.

Although... I just realized that the topmost half of the `_Fract`s in your configuration is all padding. That might make things a bit more complicated for things like signed types... Ideally the sign bit would be extended into the padding for those.


Repository:
  rC Clang

https://reviews.llvm.org/D46915