[ConstantFolding] Fix wrong folding of intrinsic 'convert.from.fp16'.

[andreadb]

Summary

This patch fixes a wrong constant folding of intrinsic 'convert.from.fp16'.

Function 'ConstantFoldScalarCall' (in ConstantFolding.cpp) works under the wrong assumption that a call to 'convert.from.fp16' always returns a value of type 'float'.

However, intrinsic 'convert.from.fp16' can be 'overloaded'; for example, we can use 'convert.from.fp16.fp64' to convert from half to double; etc.

Before this patch, the following example would have triggered an assertion failure in opt (with '-constprop'):

define double @foo() {
  entry:
  %0 = call double @llvm.convert.from.fp16.f64(i16 0)
  ret double %0
}

in Value.cpp:325: void llvm::Value::replaceAllUsesWith(llvm::Value*): Assertion `New->getType() == getType() && "replaceAllUses of value with new value of different type!" failed.

This patch fixes the problem in ConstantFolding.cpp. When folding a call to convert.from.fp16, we perform a different kind of conversion based on the call return type.

Added test 'Transform/ConstProp/convert-from-fp16.ll'.

Please let me know if ok to submit.

Thanks!
-Andrea

[ab]

LGTM, thanks!

-Ahmed

lib/Analysis/ConstantFolding.cpp

1403 ↗

(On Diff #25776)

I swear I saw the same construct elsewhere, but I can't find it. Is there a good header where this could go?

test/Transforms/ConstProp/convert-from-fp16.ll

3–4 ↗

(On Diff #25776)

Since this means the code was untested before, what using a non-0 value? For the extension, the significand of the result should look the same, so we can get creative. (but there's no point in testing APFloat again; it's just that 0 is easy to get right the wrong way)

7 ↗

(On Diff #25776)

CHECK-LABEL?

[ab]

lib/Analysis/ConstantFolding.cpp

1403 ↗

(On Diff #25776)

Ah, there's Type::getFltSemantics, please use that instead of re-defining it.

[andreadb]

Thanks for the review!

lib/Analysis/ConstantFolding.cpp

1403 ↗

(On Diff #25776)

You are right!
I should have use getFltSemantics() from Type.h.
I will use it :-).

test/Transforms/ConstProp/convert-from-fp16.ll

3–4 ↗

(On Diff #25776)

Right, I can add more tests. My original test case had something like this:

%0 = call i16 @llvm.convert.to.fp16.f64(double 5.25)
%1 = call double @llvm.convert.from.fp16.f64(i16 %0)
ret double %1

I can add more tests like the above example.
I'll send a new patch soon.

7 ↗

(On Diff #25776)

I will add CHECK-LABELs.

[andreadb]

Hi Ahmed,
I updated the patch based on your previous feedback.

This patch is much simpler now. As you suggested, I also added extra tests for values different than zero.

Please let me know what you think.
Thanks!
-Andrea

[ab]

Better indeed, go ahead!

-Ahmed

lib/Analysis/ConstantFolding.cpp

1549 ↗

(On Diff #25786)

Nit: I'd put the Ty->getFltSemantics() here instead of in the variable, but I'm fine with both ways.

llvm/trunk/lib/Analysis/ConstantFolding.cpp

if (ConstantInt *Op = dyn_cast<ConstantInt>(Operands[0])) {
       case Intrinsic::bswap:
         return ConstantInt::get(Ty->getContext(), Op->getValue().byteSwap());
       case Intrinsic::ctpop:
         return ConstantInt::get(Ty, Op->getValue().countPopulation());
       case Intrinsic::convert_from_fp16: {
         APFloat Val(APFloat::IEEEhalf, Op->getValue());
 
         bool lost = false;
-        APFloat::opStatus status =
-          Val.convert(APFloat::IEEEsingle, APFloat::rmNearestTiesToEven, &lost);
+        APFloat::opStatus status = Val.convert(
+            Ty->getFltSemantics(), APFloat::rmNearestTiesToEven, &lost);
 
         // Conversion is always precise.
         (void)status;
         assert(status == APFloat::opOK && !lost &&
                "Precision lost during fp16 constfolding");
 
         return ConstantFP::get(Ty->getContext(), Val);
       }

llvm/trunk/test/Transforms/ConstProp/convert-from-fp16.ll

+; RUN: opt -constprop -S < %s | FileCheck %s
+
+; Verify that we don't crash with an assertion failure when constant folding
+; a call to intrinsic 'convert.from.fp16' if the return type is not 'float'.
+
+define float @fold_from_fp16_to_fp32() {
+; CHECK-LABEL: @fold_from_fp16_to_fp32
+; CHECK: ret float 0.000000e+00
+entry:
+  %0 = call float @llvm.convert.from.fp16.f32(i16 0)
+  ret float %0
+}
+
+define double @fold_from_fp16_to_fp64() {
+; CHECK-LABEL: @fold_from_fp16_to_fp64
+; CHECK: ret double 0.000000e+00
+entry:
+  %0 = call double @llvm.convert.from.fp16.f64(i16 0)
+  ret double %0
+}
+
+define x86_fp80 @fold_from_fp16_to_fp80() {
+; CHECK-LABEL: @fold_from_fp16_to_fp80
+; CHECK: ret x86_fp80 0xK00000000000000000000
+entry:
+  %0 = call x86_fp80 @llvm.convert.from.fp16.f80(i16 0)
+  ret x86_fp80 %0
+}
+
+define fp128 @fold_from_fp16_to_fp128() {
+; CHECK-LABEL: @fold_from_fp16_to_fp128
+; CHECK: ret fp128 0xL00000000000000000000000000000000
+entry:
+  %0 = call fp128 @llvm.convert.from.fp16.f128(i16 0)
+  ret fp128 %0
+}
+
+define ppc_fp128 @fold_from_fp16_to_ppcfp128() {
+; CHECK-LABEL: @fold_from_fp16_to_ppcfp128
+; CHECK: ret ppc_fp128 0xM00000000000000000000000000000000
+entry:
+  %0 = call ppc_fp128 @llvm.convert.from.fp16.ppcf128(i16 0)
+  ret ppc_fp128 %0
+}
+
+define float @fold_from_fp16_to_fp32_b() {
+; CHECK-LABEL: @fold_from_fp16_to_fp32_b
+; CHECK: ret float 4.000000e+00
+entry:
+  %0 = call i16 @llvm.convert.to.fp16.f64(double 4.0)
+  %1 = call float @llvm.convert.from.fp16.f32(i16 %0)
+  ret float %1
+}
+
+define double @fold_from_fp16_to_fp64_b() {
+; CHECK-LABEL: @fold_from_fp16_to_fp64_b
+; CHECK: ret double 4.000000e+00
+entry:
+  %0 = call i16 @llvm.convert.to.fp16.f64(double 4.0)
+  %1 = call double @llvm.convert.from.fp16.f64(i16 %0)
+  ret double %1
+}
+
+define x86_fp80 @fold_from_fp16_to_fp80_b() {
+; CHECK-LABEL: @fold_from_fp16_to_fp80_b
+; CHECK: ret x86_fp80 0xK40018000000000000000
+entry:
+  %0 = call i16 @llvm.convert.to.fp16.f64(double 4.0)
+  %1 = call x86_fp80 @llvm.convert.from.fp16.f80(i16 %0)
+  ret x86_fp80 %1
+}
+
+define fp128 @fold_from_fp16_to_fp128_b() {
+; CHECK-LABEL: @fold_from_fp16_to_fp128_b
+; CHECK: ret fp128 0xL00000000000000004001000000000000
+entry:
+  %0 = call i16 @llvm.convert.to.fp16.f64(double 4.0)
+  %1 = call fp128 @llvm.convert.from.fp16.f128(i16 %0)
+  ret fp128 %1
+}
+
+define ppc_fp128 @fold_from_fp16_to_ppcfp128_b() {
+; CHECK-LABEL: @fold_from_fp16_to_ppcfp128_b
+; CHECK: ret ppc_fp128 0xM40100000000000000000000000000000
+entry:
+  %0 = call i16 @llvm.convert.to.fp16.f64(double 4.0)
+  %1 = call ppc_fp128 @llvm.convert.from.fp16.ppcf128(i16 %0)
+  ret ppc_fp128 %1
+}
+
+
+declare i16 @llvm.convert.to.fp16.f64(double)
+declare float @llvm.convert.from.fp16.f32(i16)
+declare double @llvm.convert.from.fp16.f64(i16)
+declare x86_fp80 @llvm.convert.from.fp16.f80(i16)
+declare fp128 @llvm.convert.from.fp16.f128(i16)
+declare ppc_fp128 @llvm.convert.from.fp16.ppcf128(i16)