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

[John McCall via Phabricator via cfe-commits]

rjmccall added inline comments.


================
Comment at: include/clang/AST/ASTContext.h:1954
+  llvm::APInt getFixedPointMin(QualType Ty) const;
+  llvm::APInt getFixedPointOne(QualType Ty) const;
 
----------------
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.


================
Comment at: lib/CodeGen/CGExprScalar.cpp:768
+    if (CGF.getContext().getTargetInfo().unsignedFixedPointTypesHavePadding() &&
+        Ty->isUnsignedFixedPointType()) {
+      unsigned NumBits = CGF.getContext().getTypeSize(Ty);
----------------
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.


================
Comment at: lib/CodeGen/CGExprScalar.cpp:1801
+      return Builder.CreateLShr(
+          EmitScalarConversion(Val, SrcTy, DestTy, CE->getExprLoc()), scale);
+  }
----------------
ebevhan wrote:
> rjmccall wrote:
> > Please abstract functions for doing this kind of arithmetic instead of inlining them into scalar emission.  Feel free to make a new CGFixedPoint.h header and corresponding implementation file.
> > 
> > Also, it looks like you're assuming that the output type is the same size as the input type.  I don't think that's actually a language restriction, right?
> I think that EmitScalarConversion will do 'the right thing' since it's being passed an input type of width N and converting it to a type of width M, but it's doing so 'stupidly' and isn't really aware of that one of them is a fixed-point type.
> 
> I'd much rather see the conversions be completely separate from EmitScalarConversion, implemented with explicit IR operations. Or have them moved to EmitScalarConversion completely. We've done the latter.
Yeah, I agree that using EmitScalarConversion as a subroutine here is really confusing, even if somehow it works.


Repository:
  rC Clang

https://reviews.llvm.org/D48456