[PATCH] D48456: [Fixed Point Arithmetic] Casting between fixed point types and other arithmetic types

[Bevin Hansson via Phabricator via cfe-commits]

ebevhan added inline comments.


================
Comment at: lib/CodeGen/CGExprScalar.cpp:768
+    if (CGF.getContext().getTargetInfo().unsignedFixedPointTypesHavePadding() &&
+        Ty->isUnsignedFixedPointType()) {
+      unsigned NumBits = CGF.getContext().getTypeSize(Ty);
----------------
rjmccall wrote:
> ebevhan wrote:
> > rjmccall wrote:
> > > ebevhan wrote:
> > > > rjmccall wrote:
> > > > > Can you explain the padding thing?  Why is padding uniformly present or absent on all unsigned fixed point types on a target, and never on signed types?  Is this "low bits set" thing endian-specific?
> > > > I'll give an example with a fract type. Let's say fract is 16 bits wide. That means the signed fract will have a scale of 15 (15 fractional bits) and one sign bit, for a value range of [-1.0, 1.0). The LSB is worth 2^-15 and the MSB is worth  -2^0, similar to  signed integers.
> > > > 
> > > > The unsigned fract cannot be negative, but must also have a value range of 1.0. This means that either:
> > > > * The scale remains the same (15), and the MSB is a padding bit. This bit would normally be the 2^0 bit, but since the range of the number cannot exceed 1, this bit will always be 0. In this case, the LSB is worth 2^-15 (same as the signed) and the MSB is not worth anything.
> > > > * The scale is that of signed fract + 1 (16). All bits of the number are value bits, and there is no padding. The LSB is worth 2^-16 and the MSB is worth 2^-1. There is no bit with a magnitude of 2^0.
> > > > 
> > > > These rules also apply to accum types, since the number of integral bits in the unsigned accum types may not exceed that of the signed ones. Therefore, we either have to choose between having the same number of fractional bits as the signed ones (leaving the MSB as 0/padding) or having one more fractional bit than the signed ones.
> > > > 
> > > > The lowBits is just there to construct a mask to block out the MSB. There should be no endian consideration.
> > > > I'll give an example with a fract type. Let's say fract is 16 bits wide. That means the signed fract will have a scale of 15 (15 fractional bits) and one sign bit, for a value range of [-1.0, 1.0). The LSB is worth 2^-15 and the MSB is worth -2^0, similar to signed integers.
> > > 
> > > Ah, I see, and now I've found where this is laid out in the spec, as well as where the spec leaves this representation question open for unsigned types.
> > > 
> > > Is this review debating whether or not to use a padded implementation?  Do we not have any requirement to maintain compatibility with other compilers?  Do other compilers differ on the representation, perhaps by target, or is there general agreement?
> > > 
> > > In the abstract, I think it would be better to fully use all the bits available, even if it means conversions are non-trivial.
> > We had some discussions about representation in earlier patches. The target specification was a lot more free in the first revisions, but we locked it down to fewer parameters (scale options for accum types, fract scale being width-1, and the unsigned-signed scale flag) to make the implementation simpler.
> > 
> > I argued for adding the target option to set the unsigned scale to the same as the signed; Leonard's original design did not have it. The reason is simply that we use that representation downstream in our DSP-C implementation, and things would be harder to maintain for us if the upstream implementation did not support it. The only other Embedded-C implementation I know of is the one in avr-gcc, and I believe it uses all the bits for unsigned.
> > 
> > I've given some arguments for why our representation is better from an implementation perspective (conversion is easier, precision ranges are more uniform, hardware operations that operate on signed can be reused for unsigned) in previous patches, but I suspect that using all bits is a reasonable default.
> I'd be happy to read those discussions instead of asking you to summarize them if you wouldn't mind digging out a link.
> 
> That said, I agree that the right approach for Clang as a project is to allow the target to decide.  If there were really an arbitrary number of implementations, that would be different, but from the spec it seems clear that there are exactly two reasonable choices.
I think most of the discussion about representation is spread out in D46915, probably some here:
* https://reviews.llvm.org/D46915#inline-419405
* https://reviews.llvm.org/D46915#inline-421591


Repository:
  rC Clang

https://reviews.llvm.org/D48456