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

Wed Sep 23 03:36:53 PDT 2020

ebevhan added inline comments.

================
Comment at: llvm/include/llvm/ADT/APFixedPoint.h:67

+  bool canAccommodateFloatSemantics(const fltSemantics &FloatSema) const;
+
----------------
rjmccall wrote:
> ebevhan wrote:
> > rjmccall wrote:
> > > ebevhan wrote:
> > > > 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".
> > > Do you just want a method on FixedPointSemantics that returns the unscaled, i.e. "value as an integer" semantics?
> > 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.
> 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 there was a direct use for such a semantic, it might be good to add it, but I don't see how it is.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D85961