[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#1284213, @rjmccall wrote:

> >> 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.
>
> Of course it's a behavioral specification.  All I'm saying is that the implementation detail of how we perform these operations isn't really reasonable or appropriate to express in the AST.  I know it's a bit fuzzy because of some of the things we do with e.g. opaque values and so on, but the AST is not meant to be a completely implementation-defined IR; it tries to capture the formal semantics of the program including "official" conversions and so on.  Integer addition is specified as converting its arguments to a common type and then performing a homogenous operation in that type.  Fixed-point addition is not; it's specified as doing a heterogenous operation on the original (well, rvalue-converted) operand types.


Okay, those are good points. I guess I might have been a bit too focused on reusing the behavior of the AST to fit the implementation, but that doesn't seem to be the intention with it.

> 
> 
>>>>>> 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.
> 
> Not out of line with other features that significantly break with what's expressible.  But the easy alternative to storing the intermediate "type" in the AST is to just provide a global function that can compute it on demand.

Yeah, it might just be simpler to go the route of not storing the computation semantic in the AST, at least for now. That's fairly similar to the solution in the patch, so I feel a bit silly for just going around in a circle.

To make that work I think the patch needs some changes, though. There should be a function in FixedPointSemantics to find the common full-precision semantic between two semantics, and the `getFixedPointSemantics` in ASTContext should be amended to take integer types (or another method should be provided for this, but that one already exists). I think that the `FixedPointConversions` method should also be embedded into the rest of `UsualArithmeticConversions` as there shouldn't be any need to have it separate. You still want to do the rvalue conversion and other promotions, and the rules for fixed-point still fit into the arithmetic conversions, even in the spec.

`EmitFixedPointConversion` should be changed to take FixedPointSemantics rather than QualType. This has a couple of downsides:

- It can no longer handle floating point conversions. They would have to be handled in EmitScalarConversion.
- Conversion from integer is fine, but conversion to integer cannot be generalized away with the fixed-point semantics as they are currently, as that kind of conversion must round towards zero. This requires a rounding step for negative numbers before downscaling, which the current code does not do.

Is there a better way of generalizing this?

All `EmitFixedPointAdd` should have to do is get the semantics of the operands and result type, get their full-precision semantic, call `EmitFixedPointConversion` on both operands, do the addition, and call it again to convert back to the result value. Move as much of the conversions as possible out of the function.

Does all this sound reasonable?


Repository:
  rC Clang

https://reviews.llvm.org/D53738