[PATCH] D53738: [Fixed Point Arithmetic] Fixed Point Addition
John McCall via Phabricator via cfe-commits
cfe-commits at lists.llvm.org
Mon Nov 5 22:28:34 PST 2018
rjmccall added a comment.
In https://reviews.llvm.org/D53738#1287123, @ebevhan wrote:
> In https://reviews.llvm.org/D53738#1284213, @rjmccall wrote:
> > 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?
`FixedPointSemantics` is free to do whatever is convenient for the representation; if it helps to make it able to explicitly represent an integral type — and then just assert that it's never in that form when it's used in certain places, I think that works. Although you might consider making a `FixedPointOrIntegralSemantics` class and then making `FixedPointSemantics` a subclass which adds nothing to the representation but semantically asserts that it's representing a fixed-point type.
> 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?
Makes sense to me.
More information about the cfe-commits