[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: include/clang/AST/ASTContext.h:1954
+  llvm::APInt getFixedPointMin(QualType Ty) const;
+  llvm::APInt getFixedPointOne(QualType Ty) const;
 
----------------
rjmccall wrote:
> ebevhan wrote:
> > rjmccall wrote:
> > > Are these opaque bit-patterns?  I think there should be a type which abstracts over constant fixed-point values, something `APSInt`-like that also carries the signedness and scale.  For now that type can live in Clang; if LLVM wants to add intrinsic support for fixed-point, it'll be easy enough to move it there.
> > All of the parameters that determine these values (min, max, one) need to be sourced from the target, though. It's not like APSInt is aware of the dimensions of integers on the targets. I think these should return APSInt and not APInt, though.
> > 
> > Creating another abstraction on top of APInt is probably still quite useful, especially for certain operations (multiplication, division, saturating conversion, saturating operations). Probably not too useful for this patch since it doesn't deal with constant evaluation.
> Well, I think we can add the abstraction — in a separate patch that this can depend on — even if it doesn't actually provide operations that would be useful for constant-folding yet, and then we can stop introducing assumptions like this that constant values are passed around as `APInt`s.
I suppose that's reasonable. I'm unsure of what operations exactly to expose (and how to expose them without confusing potential users) but that can probably be discussed in that patch.


================
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:
> > > 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.


Repository:
  rC Clang

https://reviews.llvm.org/D48456