[PATCH] D53738: [Fixed Point Arithmetic] Fixed Point Addition

[Bevin Hansson via Phabricator via cfe-commits]

ebevhan added a comment.

In https://reviews.llvm.org/D53738#1283983, @rjmccall wrote:

> In https://reviews.llvm.org/D53738#1283861, @ebevhan wrote:
>
> > In https://reviews.llvm.org/D53738#1283459, @rjmccall wrote:
> >
> > > Well, it could be passed around through most code as some sort of abstractly-represented intermediate "type" which could be either a QualType or a fixed-point semantics.
> >
> >
> > Sounds to me like you're describing a new `Type` that can contain an arbitrary fixed-point semantic :)
>
>
> The fact that it doesn't exist in the modeled type system is important.
>
> The arbitrary-type overflow intrinsics have this same problem, and we did not try to solve it by creating a new type class for arbitrary-precision integers and promoting the arguments.


Okay, I had a look at how those were emitted, I wasn't familiar with it until now. That's certainly a way of approaching this implementation as well, though I think the full precision info should still be in the AST somewhere rather than implied from the operand types until CodeGen.

> 
> 
>> It still feels like this just introduces inconsistencies into the form of the AST. If we do add this extra type object to the BO, won't people wonder why we use ImplicitCastExpr for non-fixedpoint operations but use the special `QualTypeOrFPSemantics BinaryOperator::ComputationType;` for fixedpoint operations even though they both have nearly the same semantic meaning (converting to a common type before doing the operation)?
> 
> 
> 
> 1. You would have an inconsistency in either case, since e.g. numeric + otherwise always returns the same type as its operands, but this would not.

True, but the CompoundAssignOperator already has that inconsistency with ComputationResultType.

> 2. The question is easily answered by pointing at the language spec.  The language does not say that the operands are promoted to a common type; it says the result is determined numerically from the true numeric values of the operands.

I guess. I just see that as a behavioral specification and not necessarily an implementation detail. It's perfectly acceptable to implement it as promotion to a common type (obviously, as that's what we are going to do), and I don't really see this as the spec putting any kind of requirement on how the implementation should be done.

> 
> 
>>>> It might just be easier to store the full-precision info in BO directly. BO might be too common to warrant the size increase, though. FixedPointSemantics can probably be optimized to only take 32 bits.
>>> 
>>> What you can definitely do is store a bit in BO saying that there's extra storage for the intermediate "type".
>> 
>> Is this similar to how ExtQuals works? How would this be implemented?
> 
> Take a look at how `DeclRefExpr` stores its various optional components.

`TrailingObjects`, then. That certainly wouldn't work if CompoundAssignOperator is still a subclass of BinaryOperator, so it would have to be folded in that case.

So the implementation would be with a `TrailingObjects<BinaryOperator, QualType, FixedPointSemantics>` where it can have 2 QualTypes and 1 FixedPointSemantics, the QualTypes being subsumed from CompoundAssignOperator.

Probably still quite a hefty amount of code that would have to be updated to make this change.

> 
> 
>>>> As a side note, comparisons are still a bit up in the air. I don't think we came to a conclusion on whether they should be done in full precision or bitwise. The spec isn't clear.
>>> 
>>> ...bitwise?
>> 
>> The spec uses different wording for the arithmetic operations and comparison operations, and it's not entirely obvious what it means. For the arithmetic operators, it has the whole section on finding the full precision common type, but for comparisons it says:
>> 
>>> 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.
>> 
>> What 'directly' means in conjunction with 'the operands are not converted' is not clear. It's reasonable to assume that it either means comparing value-wise (so, the same as finding a common type that fits all values and then comparing; though this would obviously require conversion), or perhaps 'directly' means a bitwise (representation) comparison. The latter seems a bit farfetched to me, though.
> 
> I think this is just like fixed-point arithmetic: you should do an exact numeric comparison, but there's no formal conversion because it would have to be a conversion to some concrete type, which could leave to (mandatory!) inexactness when there's no common type that expresses the full range of both operands.

This is probably the correct interpretation, I just think it could have been stated a bit clearer that they pretty much do mean the same thing for the comparison operators as for the arithmetic operators.


Repository:
  rC Clang

https://reviews.llvm.org/D53738