[PATCH] D53738: [Fixed Point Arithmetic] Fixed Point Addition
John McCall via Phabricator via cfe-commits
cfe-commits at lists.llvm.org
Tue Nov 6 10:09:14 PST 2018
rjmccall added a comment.
In https://reviews.llvm.org/D53738#1288372, @ebevhan wrote:
> In https://reviews.llvm.org/D53738#1288333, @rjmccall wrote:
> > 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.
> It might just be simpler and a bit more general to add a `DownscalingRoundsTowardZero` field to `FixedPointSemantics`, which specifies that the source value should be rounded toward zero before downscaling. Then conversion routines could handle the integer case explicitly. I'm not sure if the field would be useful for anything else, though; it has a pretty specific meaning.
> I think it's a bit odd to have `FixedPointSemantics` as a specialization of something else, since technically integers are a specialization of fixed-point values and not the other way around.
Well, it's not a specialization of "integer", it's a specialization of "fixed-point or integer". It depends on how useful it ends up being to know that something statically isn't integral.
More information about the cfe-commits