[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:19
 
+#include "llvm/ADT/APFloat.h"
 #include "llvm/ADT/APSInt.h"
----------------
Pleases just forward-declare the `APFloat` and `fltSemantics`.


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


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


================
Comment at: llvm/lib/Support/APFixedPoint.cpp:437
 
+const fltSemantics *APFixedPoint::promoteFloatSemantics(const fltSemantics *S) {
+  if (S == &APFloat::BFloat())
----------------
Can this just be `static` in this file?


================
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;
----------------
Don't you need a type that can accommodate the shifted range?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D85961