[ConstantFolding] Fix GetConstantFoldFPValue to avoid cast overflow.

[bixia]

Summary

In C++, the behavior of casting a double value that is beyond the range
of a single precision floating-point to a float value is undefined. This
change replaces such a cast with APFloat::convert to convert the value,
which is consistent with how we convert a double value to a half value.

[lebedev.ri]

Test?

[sanjoy]

This change needs a test, as Roman said.

[bixia]

Add a test case.

[bixia]

Update commit message.

[lebedev.ri]

test/Transforms/ConstProp/calls.ll

199 ↗

(On Diff #191328)

Bad test. It already is optimized.
https://godbolt.org/z/Y2DAxa

[bixia]

The problem is only reported by ubsan (undefined behavior sanitizer). My test case produces a value of 1.27e+80, which is a value between max(float) and max(double). Without ubsan, llvm opt (compiled with clang for x86) produces the correct result silently. However, since (float)the-double-value has undefined behavior by c++ language, there is no guarantee that the llvm opt produced by any compiler for any platform produces the same result. Do you have suggestion on what is a better test case?

[lebedev.ri]

In D59500#1435443, @bixia wrote:

The problem is only reported by ubsan (undefined behavior sanitizer). My test case produces a value of 1.27e+80, which is a value between max(float) and max(double). Without ubsan, llvm opt (compiled with clang for x86) produces the correct result silently. However, since (float)the-double-value has undefined behavior by c++ language, there is no guarantee that the llvm opt produced by any compiler for any platform produces the same result. Do you have suggestion on what is a better test case?

That test doesn't test any of what you have just said.
It only tests that there is no call in the output.
At the very least you probably want to actually test the output, not lack of unwanted output.

[bixia]

Modify test case to check the return value.

[sanjoy]

Lgtm

[lebedev.ri]

test/Transforms/ConstProp/calls.ll

196 ↗

(On Diff #191419)

Please add a comment as to what this testing

[sanjoy]

test/Transforms/ConstProp/calls.ll

196 ↗

(On Diff #191419)

Fwiw I think the f32_overflow suffix makes it clear what it is testing and is consistent with the rest of the file.

llvm/trunk/lib/Analysis/ConstantFolding.cpp

case 's':
       return Name == "__sinh_finite" || Name == "__sinhf_finite";
     }
   }
 }
 
 namespace {
 
 Constant *GetConstantFoldFPValue(double V, Type *Ty) {
-  if (Ty->isHalfTy()) {
+  if (Ty->isHalfTy() || Ty->isFloatTy()) {
     APFloat APF(V);
     bool unused;
-    APF.convert(APFloat::IEEEhalf(), APFloat::rmNearestTiesToEven, &unused);
+    APF.convert(Ty->getFltSemantics(), APFloat::rmNearestTiesToEven, &unused);
     return ConstantFP::get(Ty->getContext(), APF);
   }
-  if (Ty->isFloatTy())
-    return ConstantFP::get(Ty->getContext(), APFloat((float)V));
   if (Ty->isDoubleTy())
     return ConstantFP::get(Ty->getContext(), APFloat(V));
   llvm_unreachable("Can only constant fold half/float/double");
 }
 
 /// Clear the floating-point exception state.
 inline void llvm_fenv_clearexcept() {
 #if defined(HAVE_FENV_H) && HAVE_DECL_FE_ALL_EXCEPT

llvm/trunk/test/Transforms/ConstProp/calls.ll

 entry:
 ; CHECK-LABEL: @test_intrinsic_pow(
 ; CHECK-NOT: call
 ; CHECK: ret
   %0 = call double @llvm.pow.f64(double 1.500000e+00, double 3.000000e+00)
   ret double %0
 }
 
+define float @test_intrinsic_pow_f32_overflow() nounwind uwtable ssp {
+entry:
+; CHECK-LABEL: @test_intrinsic_pow_f32_overflow(
+; CHECK-NOT: call
+; CHECK: ret float 0x7FF0000000000000
+  %0 = call float @llvm.pow.f32(float 40.0, float 50.0)
+  ret float %0
+}
+
 declare double @llvm.pow.f64(double, double) nounwind readonly
+declare float @llvm.pow.f32(float, float) nounwind readonly