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

[John McCall via Phabricator via llvm-commits]

rjmccall added inline comments.


================
Comment at: llvm/include/llvm/ADT/APFixedPoint.h:202
+  /// not able to fit in the specified fixed point semantics and the overflow
+  /// parameter is specified, it is set to true.
+  static APFixedPoint getFromFloatValue(const APFloat &Value,
----------------
This should specify the behavior on infinities and NaN.


================
Comment at: llvm/lib/Support/APFixedPoint.cpp:431
+  APFloat Flt(FloatSema);
+  APFloat::opStatus S = Flt.convertFromAPInt(Val, Sema.isSigned(), RM);
+
----------------
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().


================
Comment at: llvm/lib/Support/APFixedPoint.cpp:473
+  // It is fine if this overflows to infinity even for saturating types,
+  // since we will use floating point comparisons to check for saturation.
+  APFloat ScaleFactor(std::pow(2, DstFXSema.getScale()));
----------------
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?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D85961