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

[John McCall via Phabricator via cfe-commits]

rjmccall added a comment.

>> 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.

Right, but it's the same inconsistency, which is why I'm suggesting that you treat this as an opportunity to generalize it.  Or we can avoid recording it and just make it up in IRGen.

>> 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.

>>>>> 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.


Repository:
  rC Clang

https://reviews.llvm.org/D53738