[PATCH] D85961: [Fixed Point] Add floating point methods to APFixedPoint.

[Bevin Hansson via Phabricator via llvm-commits]

ebevhan marked 2 inline comments as done.
ebevhan added inline comments.


================
Comment at: llvm/include/llvm/ADT/APFixedPoint.h:67
 
+  bool canAccommodateFloatSemantics(const fltSemantics &FloatSema) const;
+
----------------
rjmccall wrote:
> 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`?
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".


================
Comment at: llvm/lib/Support/APFixedPoint.cpp:137
+
+  APSInt MaxInt = APFixedPoint::getMax(*this).getValue();
+  APFloat F(FloatSema);
----------------
rjmccall wrote:
> 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?
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.


================
Comment at: llvm/lib/Support/APFixedPoint.cpp:437
 
+const fltSemantics *APFixedPoint::promoteFloatSemantics(const fltSemantics *S) {
+  if (S == &APFloat::BFloat())
----------------
rjmccall wrote:
> Can this just be `static` in this file?
It was originally, but I need it in codegen as well so I exported it.


================
Comment at: llvm/lib/Support/APFixedPoint.cpp:457
+  // Make sure that we are operating in a type that works with this fixed-point
+  // semantic.
+  const fltSemantics *OpSema = &FloatSema;
----------------
rjmccall wrote:
> Don't you need a type that can accommodate the shifted range?
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.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D85961/new/

https://reviews.llvm.org/D85961