leonardchan marked an inline comment as done.
leonardchan added inline comments.
================
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:
> > > > > > 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.
> Alright. I still suspect that isn't what the standard intends. You save a few
> instructions, but the comparison doesn't make sense from a value perspective.
>
> We have an externally written test suite which has a bunch of tests for both
> DSP-C and E-C, and the E-C tests don't seem to behave the way you've
> described. I've also tried to use the support in avr-gcc to determine how it
> works and it doesn't seem to do a bitwise comparison there either.
>
> We can discuss this in later patches.
>
Oh, to clarify, I mean that I am performing the shifting necessary when
comparing a fixed point to int or another fixed point. We are still comparing
the values and not the bits directly.
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
