[PATCH] D141863: [llvm][APFloat] Add NaN-in-negative-zero formats by AMD and GraphCore

[Krzysztof Drewniak via Phabricator via llvm-commits]

krzysz00 added inline comments.


================
Comment at: llvm/lib/Support/APFloat.cpp:2506
+             semantics->nanEncoding == fltNanEncoding::NegativeZero) {
+    *losesInfo = fromSemantics.nanEncoding != fltNanEncoding::NegativeZero;
+    fs = *losesInfo ? opInexact : opOK;
----------------
krzysz00 wrote:
> reedwm wrote:
> > krzysz00 wrote:
> > > reedwm wrote:
> > > > Don't we only lose info if the source of the conversion is -0?
> > > I don't think so? If I understand `losesInfo` correctly, it's for (non-rounding-related?) cases where, for `x : T`, `convert(T, convert(U, x)) != x` is possible.
> > > 
> > > So since both +0 and -0 map to +0, we've lost information.
> > > I don't think so? If I understand `losesInfo` correctly, it's for (non-rounding-related?) cases where, for `x : T`, `convert(T, convert(U, x)) != x` is possible.
> > 
> > By that logic, `losesInfo` should be false if the source of the conversion is +0.
> > 
> > E.g., in your equation `convert(T, convert(U, x)) != x`, if x is the FP32 value +0, T is FP32, and U is FP8, then the equation is false, since converting +0 to FP8 and back results in the FP32 value +0.  So `losesInfo` should be false in that case. On the other hand, if x is the FP32 value -0, the equation is true, since converting -0 to FP8 and back results in the FP32 value +0, and so `losesInfo` should be true.
> Ok, to rephrase: we know that converting f32 `Inf` or `NaN` to any of the FN APFloat formats loses info, because there's two distinct concepts that get collapsed down to one value. Similarly, in our formats, putting in a 0 from a format that isn't unsigned-zero-only also loses info, as now +0 and -0 have been collapsed down to one value and you can't tell which one you had on the way back out.
Ok, having looked at the comment, on the function again you were right, +0 should not lose info but -0 should.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D141863