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

[John McCall via Phabricator via cfe-commits]

rjmccall added a comment.

In https://reviews.llvm.org/D53738#1280193, @ebevhan wrote:

> In https://reviews.llvm.org/D53738#1279069, @rjmccall wrote:
>
> > > The important difference would be that we separate the semantics of performing the conversion and the operation. I understand that adding a new type could be a bit more involved than baking the conversion into the operator, but I really do not enjoy seeing so much implicit, implementation-specific logic encapsulated in the same AST element. Anyone who wants to look at BinaryOperators with fixed-point types will need to know all of the details of how the finding the common type and conversion is done, rather than simply "it does an addition".
> >
> > It's not just that adding a new type is "involved", it's that it's a bad solution.  Those types can't actually be expressed in the language, and changing the type system to be able to express them is going to lead to a lot of pain elsewhere.
>
>
> I did figure that it might be a bit of work to adapt other parts of the code to handle the new type, but I just prefer separating the concerns and being explicit about what work is performed. To me, the clearest way of doing that is by handling the conversions explicitly in the AST, and separately from the operators themselves. Also, not being able to deal in QualTypes for the common full precision type handling means that reusing the operator handling code in Sema won't be easy, or even possible. It would have to be computed in CreateBuiltinBinOp, since it's not possible to forward anything but QualType from the CheckXXXOperands functions.
>
> If you don't think it's a good idea I'll concede, but then there's the question of how to get the full precision semantics into the operator (if we store it there). I guess the most straightforward path is something like:
>
> - In CreateBuiltinBinOp, do the normal Check function to get the result type
> - If the result is a fixed-point type, go into a separate code path
> - Ask a method for the common FixedPointSemantics of the given LHS and RHS
> - Build the correct BinaryOperator subclass.
>
>   I need to think about this to see if our downstream implementation can be adapted to work with this design.
>
>   Compound assignments are already their own subclass, though. Unless the full precision semantic information is simply baked into the regular BinaryOperator, it would require two new subclasses: one for normal full precision operators and one for compound ones? Wouldn't adding this require new hooks and code paths in visitors, even though there's really nothing different about the operator?
>
>   The type information that CompoundAssignOperator stores today is rather similar to what a full precision operator would need, though it would need to store a FixedPointSemantics instead.


Well, maybe the cleanest solution would be to actually fold `CompoundAssignOperator` back into `BinaryOperator` and just allow `BinaryOperator` to optionally store information about the intermediate type of the computation and how the operands need to be promoted.  That information could be elided in the common cases where it's trivial, of course.

The infinite-precision rule here is still internal to an individual operator, right?  The standard's not trying to say that we should evaluate `x + y < z` by doing a comparison as if all the operands were individually infinite-precision?


Repository:
  rC Clang

https://reviews.llvm.org/D53738