ebevhan added inline comments.
================
Comment at: clang/lib/Sema/SemaExpr.cpp:1304
+ RHSTy = ResultTy;
+ }
+
----------------
rjmccall wrote:
> ebevhan wrote:
> > rjmccall wrote:
> > > leonardchan wrote:
> > > > rjmccall wrote:
> > > > > leonardchan wrote:
> > > > > > rjmccall wrote:
> > > > > > > leonardchan wrote:
> > > > > > > > rjmccall wrote:
> > > > > > > > > ebevhan wrote:
> > > > > > > > > > rjmccall wrote:
> > > > > > > > > > > leonardchan wrote:
> > > > > > > > > > > > rjmccall wrote:
> > > > > > > > > > > > > ebevhan wrote:
> > > > > > > > > > > > > > rjmccall wrote:
> > > > > > > > > > > > > > > Hmm. So adding a signed integer to an unsigned
> > > > > > > > > > > > > > > fixed-point type always converts the integer to
> > > > > > > > > > > > > > > unsigned? That's a little weird, but only
> > > > > > > > > > > > > > > because the fixed-point rule seems to be the
> > > > > > > > > > > > > > > other way. Anyway, I assume it's not a bug in
> > > > > > > > > > > > > > > the spec.
> > > > > > > > > > > > > > `handleFixedPointConversion` only calculates the
> > > > > > > > > > > > > > result type of the expression, not the calculation
> > > > > > > > > > > > > > type. The final result type of the operation will
> > > > > > > > > > > > > > be the unsigned fixed-point type, but the
> > > > > > > > > > > > > > calculation itself will be done in a signed type
> > > > > > > > > > > > > > with enough precision to represent both the signed
> > > > > > > > > > > > > > integer and the unsigned fixed-point type.
> > > > > > > > > > > > > >
> > > > > > > > > > > > > > Though, if the result ends up being negative, the
> > > > > > > > > > > > > > final result is undefined unless the type is
> > > > > > > > > > > > > > saturating. I don't think there is a test for the
> > > > > > > > > > > > > > saturating case (signed integer + unsigned
> > > > > > > > > > > > > > saturating fixed-point) in the SaturatedAddition
> > > > > > > > > > > > > > tests.
> > > > > > > > > > > > > > `handleFixedPointConversion` only calculates the
> > > > > > > > > > > > > > result type of the expression, not the calculation
> > > > > > > > > > > > > > type.
> > > > > > > > > > > > >
> > > > > > > > > > > > > Right, I understand that, but the result type of the
> > > > > > > > > > > > > expression obviously impacts the expressive range of
> > > > > > > > > > > > > the result, since you can end up with a negative
> > > > > > > > > > > > > value.
> > > > > > > > > > > > >
> > > > > > > > > > > > > Hmm. With that said, if the general principle is to
> > > > > > > > > > > > > perform the operation with full precision on the
> > > > > > > > > > > > > original operand values, why are unsigned fixed-point
> > > > > > > > > > > > > operands coerced to their corresponding signed types
> > > > > > > > > > > > > *before* the operation? This is precision-limiting
> > > > > > > > > > > > > if the unsigned representation doesn't use a padding
> > > > > > > > > > > > > bit. That seems like a bug in the spec.
> > > > > > > > > > > > > Hmm. With that said, if the general principle is to
> > > > > > > > > > > > > perform the operation with full precision on the
> > > > > > > > > > > > > original operand values, why are unsigned fixed-point
> > > > > > > > > > > > > operands coerced to their corresponding signed types
> > > > > > > > > > > > > *before* the operation? This is precision-limiting if
> > > > > > > > > > > > > the unsigned representation doesn't use a padding
> > > > > > > > > > > > > bit. That seems like a bug in the spec.
> > > > > > > > > > > >
> > > > > > > > > > > > Possibly. When the standard mentions converting from
> > > > > > > > > > > > signed to unsigned fixed point, the only requirement
> > > > > > > > > > > > involved is conservation of magnitude (the number of
> > > > > > > > > > > > integral bits excluding the sign)
> > > > > > > > > > > >
> > > > > > > > > > > > ```
> > > > > > > > > > > > when signed and unsigned fixed-point types are mixed,
> > > > > > > > > > > > the unsigned type is converted to the corresponding
> > > > > > > > > > > > signed type, and this should go without loss of
> > > > > > > > > > > > magnitude
> > > > > > > > > > > > ```
> > > > > > > > > > > >
> > > > > > > > > > > > This is just speculation, but I'm under the impression
> > > > > > > > > > > > that not as much "attention" was given for unsigned
> > > > > > > > > > > > types as for signed types since "`In the embedded
> > > > > > > > > > > > processor world, support for unsigned fixed-point data
> > > > > > > > > > > > types is rare; normally only signed fixed-point data
> > > > > > > > > > > > types are supported`", but was kept anyway since
> > > > > > > > > > > > unsigned types are used a lot.
> > > > > > > > > > > >
> > > > > > > > > > > > ```
> > > > > > > > > > > > However, to disallow unsigned fixed-point arithmetic
> > > > > > > > > > > > from programming languages (in general, and from C in
> > > > > > > > > > > > particular) based on this observation, seems overly
> > > > > > > > > > > > restrictive.
> > > > > > > > > > > > ```
> > > > > > > > > > > >
> > > > > > > > > > > > I also imagine that if the programmer needs more
> > > > > > > > > > > > precision, the correct approach would be to cast up to
> > > > > > > > > > > > a type with a higher scale. The standard seems to make
> > > > > > > > > > > > an effort to expose as much in regards to the
> > > > > > > > > > > > underlying fixed point types as possible:
> > > > > > > > > > > >
> > > > > > > > > > > > ```
> > > > > > > > > > > > it should be possible to write fixed-point algorithms
> > > > > > > > > > > > that are independent of the actual fixed-point hardware
> > > > > > > > > > > > support. This implies that a programmer (or a running
> > > > > > > > > > > > program) should have access to all parameters that
> > > > > > > > > > > > define the behavior of the underlying hardware (in
> > > > > > > > > > > > other words: even if these parameters are
> > > > > > > > > > > > implementation-defined).
> > > > > > > > > > > > ```
> > > > > > > > > > > >
> > > > > > > > > > > > So the user would at least know that unsigned types may
> > > > > > > > > > > > not have the same scale as their corresponding signed
> > > > > > > > > > > > types if the hardware handles them with different
> > > > > > > > > > > > scales.
> > > > > > > > > > > >
> > > > > > > > > > > > Also added test for signed integer + unsigned
> > > > > > > > > > > > saturating fixed point.
> > > > > > > > > > > As long as we maintain the same typing behavior, does the
> > > > > > > > > > > standard permit us to just Do The Right Thing here and do
> > > > > > > > > > > the extended arithmetic with the original unsigned
> > > > > > > > > > > operand? I'm sure there are some cases where we would
> > > > > > > > > > > produce a slightly different value than an implementation
> > > > > > > > > > > that does the coercion before the operation, but that
> > > > > > > > > > > might be permitted under the standard, and as you say, it
> > > > > > > > > > > would only affect some situations that it doesn't seem
> > > > > > > > > > > the standard has given much thought to.
> > > > > > > > > > The coercion of unsigned to signed is likely done for
> > > > > > > > > > pragmatic reasons; if you have a signed and unsigned type
> > > > > > > > > > of the same rank, operating on them together won't require
> > > > > > > > > > any 'extra bits'. This means that if your hardware has
> > > > > > > > > > native support for the operations, you won't end up in a
> > > > > > > > > > situation where you really need 33 or 34 bits (for a 32 bit
> > > > > > > > > > value) to actually perform the operation. This is one of
> > > > > > > > > > the benefits of these kinds of implicit conversions.
> > > > > > > > > >
> > > > > > > > > > It probably makes the implementation a bit easier as well,
> > > > > > > > > > since you don't have to consider cases where both sides
> > > > > > > > > > have different signedness. The loss of the extra bit of
> > > > > > > > > > precision from the unsigned operand is probably not
> > > > > > > > > > considered to be very important. As Leonard said, if you
> > > > > > > > > > wanted higher precision you can simply use the next rank of
> > > > > > > > > > types. ...Of course, they seem to have forgotten that that
> > > > > > > > > > exact issue with differing signedness also applies to
> > > > > > > > > > operations with fixed-point and integer operands.
> > > > > > > > > >
> > > > > > > > > > Personally, I think the spec could be a bit more
> > > > > > > > > > restrictive with how it handles these conversions, as the
> > > > > > > > > > way they are now makes it really hard to generate code
> > > > > > > > > > sensibly for machines that have native support for these
> > > > > > > > > > kinds of operations; in many cases you'll end up requiring
> > > > > > > > > > a couple bits too many or a couple bits too few to fit in a
> > > > > > > > > > sensible register size, and ensuring that the scales match
> > > > > > > > > > what your hardware can handle is also a challenge.
> > > > > > > > > > The coercion of unsigned to signed is likely done for
> > > > > > > > > > pragmatic reasons; if you have a signed and unsigned type
> > > > > > > > > > of the same rank, operating on them together won't require
> > > > > > > > > > any 'extra bits'. This means that if your hardware has
> > > > > > > > > > native support for the operations, you won't end up in a
> > > > > > > > > > situation where you really need 33 or 34 bits (for a 32 bit
> > > > > > > > > > value) to actually perform the operation. This is one of
> > > > > > > > > > the benefits of these kinds of implicit conversions.
> > > > > > > > >
> > > > > > > > > Yeah, I can see that. I think it really depends on the
> > > > > > > > > target. If you have hardware support, and you're using an
> > > > > > > > > unpadded representation for unsigned fixed-point values, then
> > > > > > > > > preserving the extra bit on mixed-signedness operations will
> > > > > > > > > sometimes prevent you from using the hardware optimally. On
> > > > > > > > > the other hand, if you don't have hardware support,
> > > > > > > > > preserving the extra bit is usually easier than not: e.g.,
> > > > > > > > > IIUC, multiplying a 32-bit signed _Fract with a 32-bit
> > > > > > > > > unpadded unsigned _Fract means just doing a 64-bit
> > > > > > > > > multiplication and dropping the low 32 bits, whereas doing
> > > > > > > > > the conversion first actually requires extra operations to
> > > > > > > > > ensure that the low bit doesn't contribute to the result.
> > > > > > > > > And a target with hardware support is probably going to use a
> > > > > > > > > padded format for unsigned fixed-point precisely so that it
> > > > > > > > > can take advantage of the hardware.
> > > > > > > > >
> > > > > > > > > > ...Of course, they seem to have forgotten that that exact
> > > > > > > > > > issue with differing signedness also applies to operations
> > > > > > > > > > with fixed-point and integer operands.
> > > > > > > > >
> > > > > > > > > Right.
> > > > > > > > For signed-unsigned multiplication, we could do that without
> > > > > > > > having to do the conversion first, but in the case of addition
> > > > > > > > and subtraction, we would still need to drop a bit with
> > > > > > > > unpadded unsigned types so both have the same scales.
> > > > > > > >
> > > > > > > > I don't want to make too many assumptions about hardware
> > > > > > > > support, but I imagine that if one supports 32 bit signed
> > > > > > > > addition, and unsigned types did not have padding, it would be
> > > > > > > > better instead to LSHR on unsigned fixed point type then do 32
> > > > > > > > bit signed addition rather than SEXT+SHL on the signed fixed
> > > > > > > > point type then do 33 bit signed addition.
> > > > > > > > [I}n the case of addition and subtraction, we would still need
> > > > > > > > to drop a bit with unpadded unsigned types so both have the
> > > > > > > > same scales.
> > > > > > >
> > > > > > > Right, but I don't think it matters because you're dropping that
> > > > > > > bit either way: with the specified semantics, you do the
> > > > > > > conversion beforehand which drops the bit, and with
> > > > > > > full-precision semantics, the bit isn't going to be representable
> > > > > > > and dropping it during the computation is equivalent to either
> > > > > > > rounding down (addition) or up (subtraction), either of which is
> > > > > > > allowed.
> > > > > > >
> > > > > > > > I don't want to make too many assumptions about hardware
> > > > > > > > support, but I imagine that if one supports 32 bit signed
> > > > > > > > addition, and unsigned types did not have padding, it would be
> > > > > > > > better instead to LSHR on unsigned fixed point type then do 32
> > > > > > > > bit signed addition rather than SEXT+SHL on the signed fixed
> > > > > > > > point type then do 33 bit signed addition.
> > > > > > >
> > > > > > > My point is just that (1) the 33-bit signed addition can just be
> > > > > > > a 32-bit signed addition given that you ultimately have to
> > > > > > > produce a 32-bit result and (2) targets with hardware support are
> > > > > > > unlikely to use an unpadded unsigned type anyway. Although I
> > > > > > > suppose (2) is a bit questionable because hardware support could
> > > > > > > be added well after an ABI has been settled on.
> > > > > > Ok. So should anything be changed here then if both semantics work?
> > > > > > Currently we still do the 32 bit addition for signed and unsigned
> > > > > > types.
> > > > > Well, most of what we're doing here is trying to lay good groundwork
> > > > > for all operations, and "both semantics work" is only true for
> > > > > same-rank addition, which is why I'm asking whether we should
> > > > > consider what semantics we actually want in general.
> > > > I think the semantics for all binary arithmetic operators (+, -, *, /)
> > > > can be simplified to:
> > > >
> > > > 1. If one operand is a signed fixed point type and the other an
> > > > unsigned type, convert the unsigned type to a signed type (which may
> > > > involve reducing the scale).
> > > > 2. Find the common type of both operands that can hold both the larger
> > > > scale and magnitude of the 2 operands, and cast them to this common
> > > > type.
> > > > 3. Perform the binary operation.
> > > > 4. Cast to result type.
> > > >
> > > > I think the semantics for addition are already settled with these
> > > > semantics, and I believe subtraction shares the same semantics as those
> > > > for addition. As you said before, the extra bit would not even matter
> > > > with full precision since it would be trimmed anyway when converting
> > > > from the common type to the result, and rounding can be either up or
> > > > down.
> > > >
> > > > Signed integer addition/subtraction with an unsigned fixed point type
> > > > with these semantics will only lead to undefined behavior if the
> > > > difference in the values between both types is negative since a
> > > > negative value cannot be represented by an unsigned fixed point type
> > > > (unless the result is saturating).
> > > >
> > > > For multiplication and division, the result could technically be
> > > > calculated also without having to cast between both sides, although for
> > > > using the fixed point mul/div intrinsics, both operands would need to
> > > > be casted to a common type with the same scale anyway. If a bit does
> > > > end up getting dropped when casting an operand from unsigned to signed
> > > > in (1), that dropped bit would not be representable when casting to the
> > > > result anyway and the standard still allows us to return a rounded
> > > > result.
> > > >
> > > > When multiplying or dividing between a fixed point and integer though,
> > > > we could just perform a regular integer mul/div if the result is
> > > > non-saturating without having to perform the steps above. For
> > > > saturating mul/div with an int, it might be easier to cast both sides
> > > > to a common type then call the saturating fixed point mul/div
> > > > intrinsics. In either case, I do not think rounding will be an issue
> > > > here since we can still calculate the result precisely and the result
> > > > will be either rounded up or down.
> > > >
> > > > These semantics do not apply to comparison/relational operands, where
> > > > if we do compare operands of different types or ranks, we would just
> > > > need to find the common type between the 2 and perform the comparison.
> > > > As you said before, the extra bit would not even matter with full
> > > > precision since it would be trimmed anyway when converting from the
> > > > common type to the result, and rounding can be either up or down.
> > >
> > > Note that this is only true if the unsigned value has a scale that's
> > > greater than or equal to the scale of the result type, which is true for
> > > same-rank addition but not necessarily with mixed-rank.
> > >
> > > > For multiplication and division, the result could technically be
> > > > calculated also without having to cast between both sides, although for
> > > > using the fixed point mul/div intrinsics, both operands would need to
> > > > be casted to a common type with the same scale anyway. If a bit does
> > > > end up getting dropped when casting an operand from unsigned to signed
> > > > in (1), that dropped bit would not be representable when casting to the
> > > > result anyway and the standard still allows us to return a rounded
> > > > result.
> > >
> > > In a `_Fract` multiplication (or a mixed-rank multiplication), the extra
> > > bit of precision can affect higher bits in the result. Consider an 8-bit
> > > format where you're multiplying unsigned `000.00001` with signed
> > > `+010.0000`: if you do this multiplication in full precision, the result
> > > is `+000.0001`, but if you do the conversion first, the result is `0`.
> > >
> > > On some level, it just seems really strange to me that the spec calls for
> > > doing arbitrary-precision arithmetic and then converting to the result
> > > type for every situation except a signed/unsigned mismatch.
> > Wait, wait... You might be on to something.
> >
> > The spec says in 4.1.4:
> > > In order to get useful and attainable results, the usual arithmetic
> > > conversions do not apply to the combination of an integer type and a
> > > fixed-point type, or the combination of two fixed-point types.
> > > In these cases:
> > > 1. the result of the operation is calculated using the values of the two
> > > operands, with their full precision;
> > > 2. if one operand has signed fixed-point type and the other operand has
> > > unsigned fixed-point type, then the unsigned fixed-point operand is
> > > converted to its corresponding signed fixed-point type and the resulting
> > > type is the type of the converted operand;
> > > 3. the result type is the type with the highest rank, [...]
> >
> > (1) comes before (2); not after. In other words, shouldn't the true result
> > of the operation be computed **before** the unsigned-signed correction?
> > Point 2 is just worded really oddly, with 'operand is converted' and
> > 'resulting type'.
> >
> > It says that "the unsigned fixed-point operand is converted to its
> > corresponding signed fixed-point type and the resulting type is the type of
> > the converted operand", but this doesn't mean that the resulting converted
> > operand is actually used, only its type is. This means that for the purpose
> > of result type calculation in (3), the converted operand type from (2) will
> > be used as that operand's type, rather than the unsigned type itself. Or?
> >
> > The wording here is very hard to interpret.
> Sure, I think you could make an argument for (2) only being meant to
> participate in determining the result type. At the very least, that seems
> more consistent, especially with the behavior for fixed-point + integer
> operations. Do you have any existing contact with the committee?
> Do you have any existing contact with the committee?
I don't, no. Do you know who to contact regarding this sort of thing?
For the record, I just tested this with avr-gcc, which supports the E-C
fixed-point (without padding on unsigned):
```
// 0.00390625 is 2*-8.
short _Accum calc = (unsigned short _Accum)0.00390625uk + (signed short
_Accum)2.0k;
```
This produces:
```
calc:
.byte 0
.byte 1
```
If the unsigned was converted to signed (Q8.7) first, it should have had its
bit stripped.
Repository:
rC Clang
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D53738/new/
https://reviews.llvm.org/D53738
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
