Page MenuHomePhabricator

[Fixed Point] Add floating point methods to APFixedPoint.
ClosedPublic

Authored by ebevhan on Aug 14 2020, 2:50 AM.

Details

Summary

This adds methods for APFixedPoint for converting to and
from floating point values.

Diff Detail

Event Timeline

ebevhan created this revision.Aug 14 2020, 2:50 AM
ebevhan requested review of this revision.Aug 14 2020, 2:50 AM
ebevhan updated this revision to Diff 287035.Aug 21 2020, 7:49 AM

Added comments and more tests.

@gousemoodhin I just want to check; are you working on or planning to work on float-fixed support in the near future? Floating point conversions are the next step for the fixed-point support, so if you aren't immediately looking at it, I would like to take a shot at it.

If you want a suggestion for something fixed-point related to work on, there's a bug I filed a while back here: https://bugs.llvm.org/show_bug.cgi?id=46554

@gousemoodhin I just want to check; are you working on or planning to work on float-fixed support in the near future? Floating point conversions are the next step for the fixed-point support, so if you aren't immediately looking at it, I would like to take a shot at it.

If you want a suggestion for something fixed-point related to work on, there's a bug I filed a while back here: https://bugs.llvm.org/show_bug.cgi?id=46554

@ebevhan
I have not yet started the task (Implementation of Floating-point to Fixed-point conversion). If you would like to start implementation, you can start. I will contribute too.

Regarding Bug: https://bugs.llvm.org/show_bug.cgi?id=46554
I will resolve it.

ebevhan updated this revision to Diff 287990.Aug 26 2020, 8:31 AM

Rebased and did minor adjustments.

ebevhan retitled this revision from Add floating point methods to APFixedPoint. to [Fixed Point] Add floating point methods to APFixedPoint..Aug 27 2020, 1:03 AM
ebevhan edited the summary of this revision. (Show Details)
ebevhan added reviewers: leonardchan, rjmccall.
ebevhan updated this revision to Diff 288222.Aug 27 2020, 1:06 AM

Fix comment mistake.

rjmccall added inline comments.Aug 27 2020, 1:33 AM
llvm/include/llvm/ADT/APFixedPoint.h
217

This should specify the behavior on infinities and NaN.

llvm/lib/Support/APFixedPoint.cpp
455

This can overflow the format and result in infinity. Maybe APFloat just needs a method to do this given an APInt and a binary exponent? It should be as simple as putting the bits in the right place and then calling normalize().

518

I don't understand. It must be possible to have a value that's representable in both the source floating-point type and the destination fixed-point type but not after shifting.

Maybe you can add methods on APFloat that just extract the denormalized significand and exponent?

I hadn't considered half precision as that's sort of off my radar. That does make both these conversion methods and the corresponding codegen implementation rather problematic.

Would it be completely unthinkable to "promote" calculations to a larger FP type (both here and in codegen) if the exponent bits are insufficient to hold the necessary scaling?

llvm/lib/Support/APFixedPoint.cpp
455

You're right. I originally only considered this to be a problem when the overflow resulted in a value that wouldn't be representable, but it's clearly not the case. 0.9999999999ur to _Float16 produces infinity. For all common fixed-point scales, single precision floating point and higher should be fine, though.

I'm surprised there isn't already a method to construct an APFloat from its constituent components. It's probably because there are different, incompatible formats so it is not safe to assume that an APFloat is constructed in a particular way.

518

There is frexp, but it doesn't return the mantissa as an APInt. I suppose it is possible to bitcast the mantissa out of there.

I wonder how well that works on the non-IEEE format, though.

Would it be completely unthinkable to "promote" calculations to a larger FP type (both here and in codegen) if the exponent bits are insufficient to hold the necessary scaling?

You could have the same problem with float/bfloat and a 128-bit fixed-point type, right? 128-bit integer types aren't at all ridiculous. I think this is something you should accommodate properly in your design; I don't think it's that problematic.

Would it be completely unthinkable to "promote" calculations to a larger FP type (both here and in codegen) if the exponent bits are insufficient to hold the necessary scaling?

You could have the same problem with float/bfloat and a 128-bit fixed-point type, right? 128-bit integer types aren't at all ridiculous. I think this is something you should accommodate properly in your design; I don't think it's that problematic.

Sure, for a scaling factor of 128, float doesn't work either. So for the case of float + 128-scaled fixed-point, we would need to do the FP arithmetic in double.

I just suspect that the code will be simpler and probably more efficient for a pattern of fpext->fmul->fptoint or inttofp->fmul->fptrunc rather than chopping up the floats and manually processing their bits. The former may also be easier to select, in cases where that matters.

It is probably reasonable to assume that there's always a type you can safely extend to such that the conversion is safe; it's very unlikely that someone would have a fixed-point type large enough to cause problems for double.

ebevhan updated this revision to Diff 290243.Sep 7 2020, 4:45 AM

Added a promotion mechanism to handle cases where the floating point type cannot be used to rescale the value.

ebevhan updated this revision to Diff 290265.Sep 7 2020, 5:46 AM

Rebased.

rjmccall added inline comments.Sep 7 2020, 11:36 AM
llvm/include/llvm/ADT/APFixedPoint.h
19

Pleases just forward-declare the APFloat and fltSemantics.

69

This should have a doc comment, which should clarify that precision loss is acceptable as long as it doesn't overflow. Also, "accommodate" seems like the wrong direction for this: I'd expect that a fixed-point type can "accommodate" a floating-point type if the floating-point values are representable as the fixed-point type, not the reverse. Maybe fitsInFloatSemantics?

llvm/lib/Support/APFixedPoint.cpp
138

I think there can be border cases with signed types where the maximum-magnitude negative value is unrepresentable but the maximum-magnitude positive value is.

Can you not do this check by just comparing the scale with the exponent range?

444

Can this just be static in this file?

464

Don't you need a type that can accommodate the shifted range?

ebevhan marked 2 inline comments as done.Sep 8 2020, 9:10 AM
ebevhan added inline comments.
llvm/include/llvm/ADT/APFixedPoint.h
69

Missed the doccomment by mistake.

I changed it to 'fitsInFloatSemantics'. It might be a bit misleading though, since it doesn't really check if the real value fits, but rather the value as an integer. That's why I went with something a bit more vague like "accommodate".

llvm/lib/Support/APFixedPoint.cpp
138

I was originally thinking of comparing the scale, but I came to the conclusion that comparing the scale is not enough. You could have a fixed-point semantic with a very tiny scale, but a huge integral part. That semantic might not work, even though the scale on its own fits.

I was testing with float and found that even with a scale equal to the max-exponent, both the min-integral value and max-integral value were representable (just not exactly).

For a signed 127-scale 128-bit fixed-point semantic, the max is 170141183460469231731687303715884105727, which is rounded to 170141183460469231731687303715884105728. Then, the minimum must also fit, naturally.

I'll add a min comparison for completion's sake, though.

444

It was originally, but I need it in codegen as well so I exported it.

464

Yes, canAccommodateFloatSemantics checks this. It doesn't check whether the 'real' min/max can fit in the floating point semantic; it checks whether the min/max as an integer can. That lets us know if the shifted value will fit, because the shifted value *is* the min/max as an integer.

ebevhan updated this revision to Diff 290501.Sep 8 2020, 9:10 AM

Addressed comments.

rjmccall added inline comments.Sep 21 2020, 10:11 AM
llvm/include/llvm/ADT/APFixedPoint.h
69

Do you just want a method on FixedPointSemantics that returns the unscaled, i.e. "value as an integer" semantics?

ebevhan added inline comments.Tue, Sep 22, 6:16 AM
llvm/include/llvm/ADT/APFixedPoint.h
69

How would we be informed of whether the semantic can fit in the floating point type, then? We'd still need a method that does that. It just seems like there'd be extra steps.

rjmccall added inline comments.Tue, Sep 22, 10:38 AM
llvm/include/llvm/ADT/APFixedPoint.h
69

You'd definitely still want a helper function like getUnscaledAccommodatingFloatSemantics() that you could just use consistently in these conversions. But it seems to me that there's virtue in having decomposed operations that seem independently useful, like being able to get the unscaled semantics, or being able to ask whether a floating-point type can accommodate the *scaled* range of a semantics.

If you disagree, I'm not going to insist, though.

ebevhan added inline comments.Wed, Sep 23, 3:36 AM
llvm/include/llvm/ADT/APFixedPoint.h
69

If there was a direct use for such a semantic, it might be good to add it, but I don't see how it is.

So is this patch good to go then?

This revision is now accepted and ready to land.Thu, Sep 24, 10:56 AM
This revision was landed with ongoing or failed builds.Fri, Oct 9, 1:31 AM
This revision was automatically updated to reflect the committed changes.