__builtin_isnan/__builtin_isfinite should not produce poison with -ffinite-math-only

[arsenm]

Bugzilla Link	46077
Version	trunk
OS	All
CC	@efriedma-quic,@aqjune,@LebedevRI,@zhengyang92,@RKSimon,@nunoplopes,@regehr,@zygoloid,@rotateright
Fixed by commit(s)	b89ae10

Extended Description

With -ffinite-math-only, I would intuitively expect __builtin_isnan to always return false, and __builtin_isfinite to always return true. This would allow compiling a fast and slow version of the same math library sources, where the special case paths are naturally optimized out in the fast variant of the library.

However, if I look at the IR produced and reading the LangRef, I believe these will currently return poison.

For these basic test functions:

bool test_isnan(float x) {
    return __builtin_isnan(x);
}

bool test_isfinite(float x) {
    return __builtin_isfinite(x);
}

I currently get this IR:

define hidden zeroext i1 @test_isnan(float %0) #0 {
  %2 = alloca float, align 4, addrspace(5)
  store float %0, float addrspace(5)* %2, align 4, !tbaa !5
  %3 = load float, float addrspace(5)* %2, align 4, !tbaa !5
  %4 = fcmp nnan ninf uno float %3, %3
  ret i1 %4
}

define hidden zeroext i1 @test_isfinite(float %0) #0 {
  %2 = alloca float, align 4, addrspace(5)
  store float %0, float addrspace(5)* %2, align 4, !tbaa !5
  %3 = load float, float addrspace(5)* %2, align 4, !tbaa !5
  %4 = call nnan ninf float @llvm.fabs.f32(float %3) #2
  %5 = fcmp nnan ninf one float %4, 0x7FF0000000000000
  ret i1 %5
}

The description for fast math flags states that the return value is poison if an input is a nan/inf with the corresponding flag. This is trivially violated in the isfinite case since a compare to a constant infinity.

I think the correct solution is to not emit the nnan/ninf flags when emitting these builtins.

[efriedma-quic]

I don't see anything wrong with isnan; LLVM should correctly fold it to false. (isfinite does look wrong.)

[rotateright]

I think this is another case of why we should not have FMF on fcmp.

I was working towards a FMF implementation where all FP values could carry FMF instead:
https://reviews.llvm.org/D61917
https://reviews.llvm.org/D67564

Assuming that was the right direction, a potential fix would be to add FMF attributes to arguments and/or FP loads in this example and then use those to reason that isnan/isfinite are always true/false in the optimizer?

FMF on loads is blocking progress on bug 35607.

[rotateright]

cc'ing more poison people.

[aqjune]

I think the correct solution is to not emit the nnan/ninf flags when emitting these builtins.

In this case, wouldn't test_isnan return true if the input was NaN?

%4 = fcmp uno float %3, %3 ; If %3 was NaN, %4 becomes true
ret i1 %4 ; returns true

What about lowering __builtin_isnan to simply ret i1 false if -ffinite-math-only is set? It will be a simpler solution if that's possible.

[rotateright]

Unless I've missed something in the description, the isnan example was already folded with:
https://reviews.llvm.org/rGf3ae9cc

We avoid the question of FMF on fcmp to get the isfinite example with:
https://reviews.llvm.org/rGb89ae102e6f5

Not sure if that's good enough to close this bug report. We still have the related bugs to track progress on getting rid of FMF on fcmp.

[arsenm]

I see the expected IR transforms today. However, my interpretation of the LangRef today says this is still a poison result. We could avoid it by not setting the FMF on fcmp

[arsenm]

Everything seems to work correctly today. If you use the __builtin_isnan and co, it sidesteps the issue by starting out with llvm.is.fpclass which doesn't support fast math flags. However if you perform the tests with the raw compares, I still think it's invalid as the LangRef is written. The behavior is correct with folds to true/false as appropriate, but my reading still suggests these are poison