[PATCH] D69275: Add constrained int->FP intrinsics

Wed Nov 27 08:08:47 PST 2019

craig.topper added inline comments.

================
Comment at: llvm/lib/CodeGen/SelectionDAG/LegalizeVectorTypes.cpp:2653
+    return Res;
+  }
+
----------------
uweigand wrote:
> uweigand wrote:
> > craig.topper wrote:
> > > uweigand wrote:
> > > > This confuses me again.   It seems this may generate a SINT_TO_FP -> HalfVT -> FP_ROUND -> OutVT chain, which introduces a potential double rounding that can lead to incorrect results even disregarding any constrained FP semantics ...
> > > For the only case we have tests for i64->f16. I think any integer value large enough to cause rounding when converted to f32 would be too large to represent at all in f16. Since f16's max exponent of 15 is less than the length of f32's mantissa.
> > Ah yes, you're right. So this should be fine with non-strict semantics.  
> > 
> > And for strict semantics, we should also be fine.  The int->f32 conversion can only raise an inexact exception, and this only in cases where the int->f16 conversion **should** raise an inexact.  The f32->f16 conversion due to construction of the input also can only raise an inexact exception, and again only in cases where we should have one.  Conversely, in every case where we should have an inexact exception, one (or both) of the intermediate steps will raise it.   (I think there may be cases where we get two, but that should be fine even for strict semantics.)
> Following up on myself: of course the f32->f16 conversion can also overflow, but again only in cases (and in fact in exactly in those cases) where the original int->f16 conversion should have overflowed.  So again this should be fine.
> 
> Sorry again for the confusion, I'm not really used to thinking about f16 :-)
We should probably fix the code only do this for f16. I think we could construct a vXi128->vXf32 test that would go through this and I think it would be wrong.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D69275