[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:
> > > > 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.
> Yeah, I initially had 2 general purpose ideas for handling the padding:
> 
> - Zero/sign extend them after all operations that modify the value (+, *, <<, casting to 
>  another type, ...). This way we know the sign part of a fixed point type, regardless of how much padding there is would, always extend to the padding and sign bit of the underlying scaled integer.
> - Masking only the data and sign bits before all operations that read the value and don't modify it (==, !=, <, ...). This way we only compare the non-padding bits whenever performing operations that require reading all bits in the underlying integer.
> 
> I wanted to choose the method that addressed operations fixed point types would least likely be used for, that is would these types be generally used more for arithmetic or comparisons. I chose the second method, but don't have any strong evidence to support this. This is also what the later patches implement but can be reworked.
I see what you mean... I also think it's more likely that the types will be used with arithmetic operators than comparisons. However, I don't have any evidence for it but it feels like it could be dangerous to let 'invalid' values hang around; it's safer to ensure that all values are correct after every operation. In our implementation we do the first method for our unsigned types. I wrote a small example (a FIR filter) to see what the overhead of using unsigned instead of signed was, and the x86 asm went from 20 to 23 instructions, which isn't huge. Granted, we do not use unsigned fixed-point types much (if at all) so overhead there isn't really a problem for us.

I think that one of the important benefits of the first method is that if unsigned fixed-point types with padding always have their padding cleared and the fractional bits are the same, then hardware operations for signed fixed-point types can be reused for the unsigned types.

I'm trying to come up with an example where things can go wrong if padding is not corrected after operations, but I can't seem to do it without invoking overflow, which is UB on both signed and unsigned fixed-point if I'm not mistaken. The E-C spec does not seem to mention anything explicit about what happens when you convert a negative signed fixed-point type to an unsigned one, so I think that's UB as well.


================
Comment at: lib/AST/ExprConstant.cpp:9437
+      }
+      return Success(-Value, E);
+    }
----------------
leonardchan wrote:
> ebevhan wrote:
> > This looks very suspect to me as well... This might not respect padding on types whose sign bit is not the MSB, such as _Fract. The same goes for the overflow check above.
> > 
> > I think it's quite important that we define the semantics of what padding bits should contain. Probably zeroes for unsigned types and a sexted sign bit for signed types.
> Initially I had the _Accum and their corresponding _Fract types the same widths to avoid casting between _Accums to a different sized _Fract (`ie short _Fract to short _Accum`), but after thinking about it, it might be simpler in the end to just have the _Fract widths have half as much as the _Accum width and have the fractional bits be one less the width (unless SameFBits is set in which case there will still be only 1 padding).
> 
> This would also reduce the number of knobs that would be tweaked for different targets and help cover this case by effectively ignoring the padding bits.
Yes, that sounds like a good idea. It is also the case for us that the scale of our _Fract types is the width  - 1. This makes many operations that care about sign bits trivial, and conversion isn't that bad, it's just a sign/zero extension which should be no problem so long as the type widths are sensible (_Fract to _Accum would be 8->16, 16->32, 32->64 for the respective sizes).

I guess this means dropping the FractFBits configuration values? I suppose the AccumFBits values cannot be dropped as the specification does not have any restrictions between _Accum and _Fract (I think?).


Repository:
  rC Clang

https://reviews.llvm.org/D46915