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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
