[PATCH] D28928: [ValueTracking] Implement SignBitMustBeZero correctly for sqrt.

[Eli Friedman via Phabricator via llvm-commits]

efriedma accepted this revision.
efriedma added inline comments.
This revision is now accepted and ready to land.


================
Comment at: llvm/test/Transforms/InstSimplify/floating-point-arithmetic.ll:237
+  %sqrt = call nnan nsz float @llvm.sqrt.f32(float %a)
+  %fabs = call float @llvm.fabs.f32(float %sqrt)
+  ret float %fabs
----------------
jlebar wrote:
> efriedma wrote:
> > I'm not convinced this transform is legal without an nsz flag on the fabs...
> > 
> > Patch is fine otherwise.
> I thought we talked about this above and established that, because fabs is a floating-point op and here its input comes from an nsz op, we can assume that the sign of the zero is "insignificant".  My understanding was that only if we have a non-fp op involved do things become complicated.
> 
> Maybe this would be easier on IRC?  Please ping me and Hal.
Here's what I understand how you're looking at this from this thread:

1. nsz fabs() can return -0.0.
2. bitcasting a float to an int "freezes" the sign bit (it can be either set or unset, but it's consistent).
3. Floating-point operations whose operand is, directly or indirectly, an nsz instruction, have indeterminate signed zeros.

I think 3 might be a bit too aggressive; it seems like a good idea to enforce the invariant that in non-fast-math code, fabs(x) never has the sign bit set.  With this patch, that breaks if you use a library compiled with fast-math to produce x.  

That said, mixing fast-math and non-fast-math code is likely to be pretty infrequent, so maybe we shouldn't worry about it.  Feel free to commit as-is.


https://reviews.llvm.org/D28928