[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: lib/Sema/SemaExpr.cpp:1248
+  bool RHSFixed = RHSType->isFixedPointType();
+
+  if (LHSFixed && RHSFixed) {
----------------
ebevhan wrote:
> leonardchan wrote:
> > ebevhan wrote:
> > > leonardchan wrote:
> > > > ebevhan wrote:
> > > > > I don't see how these semantics work properly. The specification requires that operations be done in the full precision of both types. You cannot convert the types before performing the operation like this, since the operation will not be done in full precision in that case.
> > > > > 
> > > > > The operator semantics of Embedded-C require the operand types of binary operators to be different. It's only when you've performed the operation that you are allowed to convert the result to the resulting type.
> > > > Initially the idea was to convert both sides to fixed point types, then perform standard binary operations between the fixed point types.
> > > > 
> > > > For the example, a `fract * int` would have the int converted to a fixed point type by left shifting it by the scale of the fract, multiplying, then right shifting by the scale again to get the resulting fract. The only unhandled thing is overflow, but the precision of the fract remains the same. The operands would also be casted up beforehand so there was enough space to store the result, which was casted down back to the original fract after performing the right shift by the scale.
> > > > 
> > > > Operations between fixed point types would follow a similar process of casting both operands to the higher rank fixed point type, and depending on the operation, more underlying shifting and casting would be done to retain full precision of the higher ranked type.
> > > > 
> > > > Though I will admit that I did not realize until now that multiplying a fixed point type by an integer does not require shifting the integer.
> > > I see how you've reasoned; this is how C normally works. The `fract` is of higher rank than `int` and therefore is the 'common type' of the operation. However, even though it is higher rank there is no guarantee that you can perform the operation without overflowing. And overflow matters here; the spec says that it must be done in the full precision (integral + fractional) of both types.
> > > 
> > > > The only unhandled thing is overflow, but the precision of the fract remains the same. The operands would also be casted up beforehand so there was enough space to store the result, which was casted down back to the original fract after performing the right shift by the scale.
> > > 
> > > The precision remains the same (and while it doesn't have to be the same to perform an operation, it makes the implementation more regular; things like addition and subtraction 'just work'), but you cannot perform a conversion to `fract` *before* the operation itself, since if you do, there's nothing to 'cast up'. Casting up is needed for things like `fract * fract` to prevent overflow, but for `fract * int` you need to cast to a type that can fit both all values of the int and all values of the fract, and *then* you can cast up before doing the multiplication.
> > > 
> > > > Operations between fixed point types would follow a similar process of casting both operands to the higher rank fixed point type, and depending on the operation, more underlying shifting and casting would be done to retain full precision of the higher ranked type.
> > > 
> > > This might work, but I feel there could be edge cases. The E-C fixed-point ranks are very odd as they don't reflect reality; `short _Accum` cannot be considered strictly 'above' `long _Fract`, but the former has a higher rank than the latter. Depending on how the types are specified for a target, implicit casts between fixed-point types might inadvertantly discard bits, even though the spec says that operations must be done in full precision.
> > I see, so just to confirm, something like a `fract * int` would not result in any implicit casting between either operand, but any special arithmetic, like intermediate storage types or saturation handling, would be handled by the underlying IR?
> > 
> > So should really no conversions/implicit type casting should be performed here and instead all handling of arithmetic operations should happen somewhere during the codegen stage?
> > 
> > I see, so just to confirm, something like a fract * int would not result in any implicit casting between either operand, but any special arithmetic, like intermediate storage types or saturation handling, would be handled by the underlying IR?
> 
> Yes, for operations which require precision that cannot be provided by any of the existing types, there must be an 'invisible' implicit conversion to a type which can represent all of the values of either operand. This conversion cannot be represented in the AST as it is today.
> 
> The simplest solution is indeed to not have any implicit cast at all in the AST and resolve these conversions when needed (CodeGen and consteval are the locations I can think of), but ultimately it feels a bit dirty... I think that the best solution AST-wise is to define a completely new type class (perhaps FullPrecisionFixedPointType) that represents a fixed-point type with arbitrary width, scale, signedness and saturation. Then you can define ImplicitCasts to an instance of this type that can fit both the `int` and the `fract`. I don't know if adding this is acceptable upstream, though.
> 
> I think all of these rules must apply to fixed-fixed operations as well; a `short accum * long fract` must be done as a type that does not exist, similar to fixed-int. It's not clear how saturation should work here either...
> 
> I also noticed now that the spec says in regards to comparison operators, `When comparing fixed-point values with fixed-point values or integer values, the values are compared directly; the values of the operands are not converted before the comparison is made.` I'm not sure what this means.
In any case, to clarify, I think there are two paths to consider. Either:

  - Add a new type class to the type system that encapsulates an arbitrary-precision fixed-point type that can be used for implicit casts when operating on fixed-point and integer types. This is in my opinion the cleaner solution, since it retains invariants on the types of operators and simplifies any logic that deals with operators; or,
  - Leave the operands of these operations uncasted. This is in some way simpler, since it doesn't require adding a whole new type, but it complicates other parts of the code. Anything that wants to deal with fixed-point operators will need to know how to do fixed-point conversion as well, which isn't a very good separation of responsibility IMO. It also breaks the C invariant of operands of arithmetic types being in a common type, which might be surprising to people.




Repository:
  rC Clang

https://reviews.llvm.org/D46915