ebevhan added inline comments.
================
Comment at: lib/Sema/SemaExpr.cpp:1248
+ bool RHSFixed = RHSType->isFixedPointType();
+
+ if (LHSFixed && RHSFixed) {
----------------
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.
================
Comment at: lib/Sema/SemaExpr.cpp:1248
+ bool RHSFixed = RHSType->isFixedPointType();
+
+ if (LHSFixed && RHSFixed) {
----------------
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.
Repository:
rC Clang
https://reviews.llvm.org/D46915
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
