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

[Justin Lebar via Phabricator via llvm-commits]

jlebar added inline comments.


================
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
----------------
efriedma wrote:
> 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.
Okay, will do, thanks.

FWIW I totally see your point, and personally I don't feel particularly strongly about this.  But I also don't think we have a particularly clear idea about how we want this to work at the moment.  So...yeah.

Thanks again.


https://reviews.llvm.org/D28928