[PATCH] D46915: [Fixed Point Arithmetic] Fixed Point Precision Bits and Fixed Point Literals

[Leonard Chan via Phabricator via cfe-commits]

leonardchan added inline comments.


================
Comment at: lib/Sema/SemaExpr.cpp:1248
+  bool RHSFixed = RHSType->isFixedPointType();
+
+  if (LHSFixed && RHSFixed) {
----------------
ebevhan wrote:
> ebevhan wrote:
> > leonardchan wrote:
> > > leonardchan wrote:
> > > > ebevhan wrote:
> > > > > 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.
> > > > > 
> > > > > 
> > > > I'm actually more of a fan for the second case. Aside, aside from the literal parsing in NumericLieralParser, wouldn't the only other place that would actually need to know about fixed point conversion be `ScalarExprEmitter` under CodeGen/CGExprScalar.cpp?
> > > > 
> > > > It seems that it's this class that creates the binary operations and other code gen classes like CodeGenFunction just make underlying calls to ScalarExprEmitter, so the actual conversion logic may just be contained here. Most of the implicit casting handled under UsualArithmeticConversions seems to be handled by `VisitCastExpr` under ScalarExprEmitter also, so adding another casting type would in the end just result in another case in the switch statement there, which in turn may just result in another call to ScalarExprEmitter.
> > > > 
> > > > I can see how it might be weird at first that these types don't fall under usual arithmetic, but the standard does specify that it wouldn't.
> > > Regarding comparison operators, my guess is that it means during comparison operations specifically, the actual underlying values of each operand are compared instead of having the special type conversions take place. That is, `1.0k != 1` but `1.0k == 128` (assuming scale of 7). If this is the case, we could actually save a few operations not having to do a shift on the integer.
> > > 
> > > I also can't seem to find a test case used by GCC  where they explicitly compare a fixed point type against an integer. Normally, they instead assign the FP literal to an integral type, then compare that against another integer.
> > > 
> > > I'm referring to `CONV_ACCUM_INT` in
> > > https://github.com/gcc-mirror/gcc/blob/e11be3ea01eaf8acd8cd86d3f9c427621b64e6b4/gcc/testsuite/gcc.dg/fixed-point/convert.h
> > > I'm actually more of a fan for the second case. Aside, aside from the literal parsing in NumericLieralParser, wouldn't the only other place that would actually need to know about fixed point conversion be ScalarExprEmitter under CodeGen/CGExprScalar.cpp?
> > 
> > ExprConstant (consteval) would also have to know, since the input expressions would be these 'unbalanced' binary operations. I'm not sure why it would affect literal parsing, though?
> > 
> > Regarding VisitCastExpr; in the first case, I'm not talking about adding a new CastKind, I'm talking about adding a whole new type altogether. This type would be just as much a fixed-point type as the builtin ones, just with a configurable width and scale. Then, something like this:
> > ```
> >   int * fract
> > ```
> > where int is 32 bits and fract is 16 bits Q15, would become
> > ```
> >  (fract)((FullPrecFixedPoint<32+16, 0+15>)int * (FullPrecFixedPoint<32+16, 0+15>)fract)
> > ```
> > The cast on the `int` is a `CK_IntegerToFixedPointCast`, and the cast on the `fract` is a `CK_FixedPointCast`. All values and operations are self-consistent and fully representable in the AST. Converting to and from a FullPrecFixedPoint type is no different from converting to and from, say, `fract`. They are both fixed-point types with width and scale; one is just built-in and the other is 'artificial'. The multiplication is performed like any other fixed-point operation, just in a higher width and (possibly higher) scale than either of the operands.
> > 
> > The issue I have with the second case is that the AST is somehow left 'unfinished'. There *are* casts there, but they are just not representable in the AST. In order to represent them, you would need to add these arbitrary-precision types.
> > Regarding comparison operators, my guess is that it means during comparison operations specifically, the actual underlying values of each operand are compared instead of having the special type conversions take place. That is, 1.0k != 1 but 1.0k == 128 (assuming scale of 7). If this is the case, we could actually save a few operations not having to do a shift on the integer.
> 
> Right... That seems incredibly dangerous to me; I really hope this isn't what the spec means. 1.0 is by no means the same thing as 128. On top of that, it means that comparisons between fixed-point and integer can vary depending on the scale of the fixed-point type; this feels really shaky to me. Heck, if SameFBits is false, for a scale of 7, `0.5r == 64`, but `0.5ur != 64`. It might even be the case that `0.5r != 0.5ur`. Absolutely bizarre, and incredibly confusing for programmers!
> 
> It might have been done this way to make it easy to inspect the raw bits of a fixed-point number, but why not just do a bit-preserving conversion and compare as an integer in that case?
> 
> DSP-C simply prohibits 'ambiguous' type conversions such as these to prevent this confusion from happening.
I also noticed that the doc provides a `bitsfx` function for getting this underlying integer value representation from a fixed point type. Still going to leave the comparisons as what I initially interpreted them as by comparing them normally (`1.0k == 1`).


================
Comment at: lib/Sema/SemaExpr.cpp:1248
+  bool RHSFixed = RHSType->isFixedPointType();
+
+  if (LHSFixed && RHSFixed) {
----------------
leonardchan wrote:
> ebevhan wrote:
> > ebevhan wrote:
> > > leonardchan wrote:
> > > > leonardchan wrote:
> > > > > ebevhan wrote:
> > > > > > 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.
> > > > > > 
> > > > > > 
> > > > > I'm actually more of a fan for the second case. Aside, aside from the literal parsing in NumericLieralParser, wouldn't the only other place that would actually need to know about fixed point conversion be `ScalarExprEmitter` under CodeGen/CGExprScalar.cpp?
> > > > > 
> > > > > It seems that it's this class that creates the binary operations and other code gen classes like CodeGenFunction just make underlying calls to ScalarExprEmitter, so the actual conversion logic may just be contained here. Most of the implicit casting handled under UsualArithmeticConversions seems to be handled by `VisitCastExpr` under ScalarExprEmitter also, so adding another casting type would in the end just result in another case in the switch statement there, which in turn may just result in another call to ScalarExprEmitter.
> > > > > 
> > > > > I can see how it might be weird at first that these types don't fall under usual arithmetic, but the standard does specify that it wouldn't.
> > > > Regarding comparison operators, my guess is that it means during comparison operations specifically, the actual underlying values of each operand are compared instead of having the special type conversions take place. That is, `1.0k != 1` but `1.0k == 128` (assuming scale of 7). If this is the case, we could actually save a few operations not having to do a shift on the integer.
> > > > 
> > > > I also can't seem to find a test case used by GCC  where they explicitly compare a fixed point type against an integer. Normally, they instead assign the FP literal to an integral type, then compare that against another integer.
> > > > 
> > > > I'm referring to `CONV_ACCUM_INT` in
> > > > https://github.com/gcc-mirror/gcc/blob/e11be3ea01eaf8acd8cd86d3f9c427621b64e6b4/gcc/testsuite/gcc.dg/fixed-point/convert.h
> > > > I'm actually more of a fan for the second case. Aside, aside from the literal parsing in NumericLieralParser, wouldn't the only other place that would actually need to know about fixed point conversion be ScalarExprEmitter under CodeGen/CGExprScalar.cpp?
> > > 
> > > ExprConstant (consteval) would also have to know, since the input expressions would be these 'unbalanced' binary operations. I'm not sure why it would affect literal parsing, though?
> > > 
> > > Regarding VisitCastExpr; in the first case, I'm not talking about adding a new CastKind, I'm talking about adding a whole new type altogether. This type would be just as much a fixed-point type as the builtin ones, just with a configurable width and scale. Then, something like this:
> > > ```
> > >   int * fract
> > > ```
> > > where int is 32 bits and fract is 16 bits Q15, would become
> > > ```
> > >  (fract)((FullPrecFixedPoint<32+16, 0+15>)int * (FullPrecFixedPoint<32+16, 0+15>)fract)
> > > ```
> > > The cast on the `int` is a `CK_IntegerToFixedPointCast`, and the cast on the `fract` is a `CK_FixedPointCast`. All values and operations are self-consistent and fully representable in the AST. Converting to and from a FullPrecFixedPoint type is no different from converting to and from, say, `fract`. They are both fixed-point types with width and scale; one is just built-in and the other is 'artificial'. The multiplication is performed like any other fixed-point operation, just in a higher width and (possibly higher) scale than either of the operands.
> > > 
> > > The issue I have with the second case is that the AST is somehow left 'unfinished'. There *are* casts there, but they are just not representable in the AST. In order to represent them, you would need to add these arbitrary-precision types.
> > > Regarding comparison operators, my guess is that it means during comparison operations specifically, the actual underlying values of each operand are compared instead of having the special type conversions take place. That is, 1.0k != 1 but 1.0k == 128 (assuming scale of 7). If this is the case, we could actually save a few operations not having to do a shift on the integer.
> > 
> > Right... That seems incredibly dangerous to me; I really hope this isn't what the spec means. 1.0 is by no means the same thing as 128. On top of that, it means that comparisons between fixed-point and integer can vary depending on the scale of the fixed-point type; this feels really shaky to me. Heck, if SameFBits is false, for a scale of 7, `0.5r == 64`, but `0.5ur != 64`. It might even be the case that `0.5r != 0.5ur`. Absolutely bizarre, and incredibly confusing for programmers!
> > 
> > It might have been done this way to make it easy to inspect the raw bits of a fixed-point number, but why not just do a bit-preserving conversion and compare as an integer in that case?
> > 
> > DSP-C simply prohibits 'ambiguous' type conversions such as these to prevent this confusion from happening.
> I also noticed that the doc provides a `bitsfx` function for getting this underlying integer value representation from a fixed point type. Still going to leave the comparisons as what I initially interpreted them as by comparing them normally (`1.0k == 1`).
Understandable. Initially I thought it was a new cast for converting each fixed point type to this intermediate type. I will add this in a new separate patch that addresses conversions involving fixed point types since I feel this one is already large enough and was meant to address just parsing fixed point literals. 


Repository:
  rC Clang

https://reviews.llvm.org/D46915