[CodeGen] Convert double -> __fp16 in one step.

[ab]

Summary

Hi,

The attached patch changes Clang so that for types bigger than float, instead of converting to fp16 via the sequence "InTy -> float -> fp16", we perform conversions in just one step. This avoids the double rounding which potentially changes results from a natural IEEE-754 operation.

There are potential problems, but I believe the benefits outweigh them:

• It's a change in semantics. I believe it's compatible with the major standards though (OpenCL requires accesses go via a builtin; n1833 for C would *demand* this change, from my reading of it).
• It means double -> __fp16 conversion will fail on x86 and v7 ARM CPUs for now. Specifically, we will generate a libcall which isn't actually widespread (or probably implemented anywhere). I think this is preferable to the status-quo of producing a possibly incorrect result though.

Longer term, I'd like to improve the codegen here to use real fpext/fptrunc operations and remove many of the special cases for half. Unfortunately the LLVM CodeGen isn't up to this change yet, so I've just extended the use of the @llvm.convert.to.fp16 intrinsic.

So, is it OK to change this?

Cheers.

Tim.

[t.p.northover]

Ping?

Cheers.

Tim.

[ab]

Hi all,

Ping for Tim's patch!

I rebased it and added the one-step __fp16->double conversion, which, while not necessary for correctness, is nice to have (if only for symmetry.)
We're still missing the libcalls, so this will break the conversions from/to double. However, I agree with Tim in that it's better than incorrect results.

-Ahmed

[ab]

Ping!

[andreadb]

Hi Ahmed,

The patch looks good to me. However, I don't think I am knowledgeable enough to give the final approval. So, somebody else should review this patch.

My personal opinion (for what is worth..) is that this is the correct approach. On X86, I am not particularly worried about the extra runtime libcalls which could be generated for double-to-half conversions. Also, (as you already know) the target independent legalizer now knows how to split a double-to-half conversion into the sequence 'double-to-float' plus 'float-to-half' (only under fast-math).

[t.p.northover]

24 February 2015 at 10:47, Ahmed Bougacha <[e-mail]> wrote:

Ping!

I still think this is the right approach too. A 2-step conversion is
pretty much impossible to justify logically; users who really still
want it can force that behaviour by casting to float first.

Cheers.

Tim.

[ab]

Ping again!

(Andrea & Tim, thanks for the comments)

-Ahmed

[ab]

Monday ping!

[ab]

Ping!

Tentatively adding Oliver as a reviewer =)

-Ahmed

[ab]

Reviewed as part of http://reviews.llvm.org/D8367

[ab]

.. and committed in r232968.

-Ahmed

lib/CodeGen/CGExprScalar.cpp

Value *ScalarExprEmitter::EmitScalarConversion(Value *Src, QualType SrcType,
   if (SrcType == DstType) return Src;
 
   if (DstType->isVoidType()) return nullptr;
 
   llvm::Value *OrigSrc = Src;
   QualType OrigSrcType = SrcType;
   llvm::Type *SrcTy = Src->getType();
 
-  // If casting to/from storage-only half FP, use special intrinsics.
+  // Handle conversions to bool first, they are special: comparisons against 0.
+  if (DstType->isBooleanType())
+    return EmitConversionToBool(Src, SrcType);
+
+  llvm::Type *DstTy = ConvertType(DstType);
+
+  // Cast from storage-only half FP using the special intrinsic.
   if (SrcType->isHalfType() && !CGF.getContext().getLangOpts().NativeHalfType &&
       !CGF.getContext().getLangOpts().HalfArgsAndReturns) {
+    if (DstTy->isFloatingPointTy())
+      return Builder.CreateCall(
+          CGF.CGM.getIntrinsic(llvm::Intrinsic::convert_from_fp16, DstTy), Src);
+
+    // If this isn't an FP->FP conversion, go through float.
     Src = Builder.CreateCall(
         CGF.CGM.getIntrinsic(llvm::Intrinsic::convert_from_fp16,
                              CGF.CGM.FloatTy),
         Src);
     SrcType = CGF.getContext().FloatTy;
     SrcTy = CGF.FloatTy;
   }
 
-  // Handle conversions to bool first, they are special: comparisons against 0.
-  if (DstType->isBooleanType())
-    return EmitConversionToBool(Src, SrcType);
-
-  llvm::Type *DstTy = ConvertType(DstType);
-
   // Ignore conversions like int -> uint.
   if (SrcTy == DstTy)
     return Src;
 
   // Handle pointer conversions next: pointers can only be converted to/from
   // other pointers and integers. Check for pointer types in terms of LLVM, as
   // some native types (like Obj-C id) may map to a pointer type.
   if (isa<llvm::PointerType>(DstTy)) {
Value *ScalarExprEmitter::EmitScalarConversion(Value *Src, QualType SrcType,
 
   // An overflowing conversion has undefined behavior if either the source type
   // or the destination type is a floating-point type.
   if (CGF.SanOpts.has(SanitizerKind::FloatCastOverflow) &&
       (OrigSrcType->isFloatingType() || DstType->isFloatingType()))
     EmitFloatConversionCheck(OrigSrc, OrigSrcType, Src, SrcType, DstType,
                              DstTy);
 
-  // Cast to half via float
+  // Cast to half using the intrinsic if from FP type, through float otherwise.
   if (DstType->isHalfType() && !CGF.getContext().getLangOpts().NativeHalfType &&
-      !CGF.getContext().getLangOpts().HalfArgsAndReturns)
+      !CGF.getContext().getLangOpts().HalfArgsAndReturns) {
+    if (SrcTy->isFloatingPointTy())
+      return Builder.CreateCall(
+          CGF.CGM.getIntrinsic(llvm::Intrinsic::convert_to_fp16, SrcTy), Src);
     DstTy = CGF.FloatTy;
+  }
 
   if (isa<llvm::IntegerType>(SrcTy)) {
     bool InputSigned = SrcType->isSignedIntegerOrEnumerationType();
     if (isa<llvm::IntegerType>(DstTy))
       Res = Builder.CreateIntCast(Src, DstTy, InputSigned, "conv");
     else if (InputSigned)
       Res = Builder.CreateSIToFP(Src, DstTy, "conv");
     else

test/CodeGen/fp16-ops.c

 // REQUIRES: arm-registered-target
 // RUN: %clang_cc1 -emit-llvm -o - -triple arm-none-linux-gnueabi %s | FileCheck %s
 typedef unsigned cond_t;
 
 volatile cond_t test;
 volatile __fp16 h0 = 0.0, h1 = 1.0, h2;
 volatile float f0, f1, f2;
+volatile double d0;
 
 void foo(void) {
   // CHECK-LABEL: define void @foo()
 
   // Check unary ops
 
   // CHECK: call float @llvm.convert.from.fp16.f32(
   // CHECK fptoi float
void foo(void) {
   // CHECK: fmul float
   // CHECK: call i16 @llvm.convert.to.fp16.f32(
   h1 = h0 * h2;
   // CHECK: call float @llvm.convert.from.fp16.f32(
   // CHECK: call i16 @llvm.convert.to.fp16.f32(
   // CHECK: call float @llvm.convert.from.fp16.f32(
   // CHECK: fmul float
   // CHECK: call i16 @llvm.convert.to.fp16.f32(
-  h1 = h0 * (__fp16) -2.0;
+  h1 = h0 * (__fp16) -2.0f;
   // CHECK: call float @llvm.convert.from.fp16.f32(
   // CHECK: fmul float
   // CHECK: call i16 @llvm.convert.to.fp16.f32(
   h1 = h0 * f2;
   // CHECK: call float @llvm.convert.from.fp16.f32(
   // CHECK: fmul float
   // CHECK: call i16 @llvm.convert.to.fp16.f32(
   h1 = f0 * h2;
 
   // CHECK: call float @llvm.convert.from.fp16.f32(
   // CHECK: call float @llvm.convert.from.fp16.f32(
   // CHECK: fdiv float
   // CHECK: call i16 @llvm.convert.to.fp16.f32(
   h1 = (h0 / h2);
   // CHECK: call float @llvm.convert.from.fp16.f32(
   // CHECK: call float @llvm.convert.from.fp16.f32(
   // CHECK: fdiv float
   // CHECK: call i16 @llvm.convert.to.fp16.f32(
-  h1 = (h0 / (__fp16) -2.0);
+  h1 = (h0 / (__fp16) -2.0f);
   // CHECK: call float @llvm.convert.from.fp16.f32(
   // CHECK: fdiv float
   // CHECK: call i16 @llvm.convert.to.fp16.f32(
   h1 = (h0 / f2);
   // CHECK: call float @llvm.convert.from.fp16.f32(
   // CHECK: fdiv float
   // CHECK: call i16 @llvm.convert.to.fp16.f32(
   h1 = (f0 / h2);
void foo(void) {
   // CHECK: call float @llvm.convert.from.fp16.f32(
   // CHECK: fsub float
   // CHECK: call i16 @llvm.convert.to.fp16.f32(
   h1 = (h2 - h0);
   // CHECK: call float @llvm.convert.from.fp16.f32(
   // CHECK: call float @llvm.convert.from.fp16.f32(
   // CHECK: fsub float
   // CHECK: call i16 @llvm.convert.to.fp16.f32(
-  h1 = ((__fp16)-2.0 - h0);
+  h1 = ((__fp16)-2.0f - h0);
   // CHECK: call float @llvm.convert.from.fp16.f32(
   // CHECK: fsub float
   // CHECK: call i16 @llvm.convert.to.fp16.f32(
   h1 = (h2 - f0);
   // CHECK: call float @llvm.convert.from.fp16.f32(
   // CHECK: fsub float
   // CHECK: call i16 @llvm.convert.to.fp16.f32(
   h1 = (f2 - h0);
void foo(void) {
   // CHECK: fcmp une
   // CHECK: call float @llvm.convert.from.fp16.f32(
   // CHECK: call float @llvm.convert.from.fp16.f32(
   // CHECK: call i16 @llvm.convert.to.fp16.f32(
   h1 = (h1 ? h2 : h0);
   // Check assignments (inc. compound)
   h0 = h1;
   // CHECK: call i16 @llvm.convert.to.fp16.f32(
-  h0 = (__fp16)-2.0;
+  h0 = (__fp16)-2.0f;
   // CHECK: call i16 @llvm.convert.to.fp16.f32(
   h0 = f0;
 
   // CHECK: call float @llvm.convert.from.fp16.f32(
   // CHECK: call float @llvm.convert.from.fp16.f32(
   // CHECK: fadd float
   // CHECK: call i16 @llvm.convert.to.fp16.f32(
   h0 += h1;
   // CHECK: call float @llvm.convert.from.fp16.f32(
   // CHECK: call float @llvm.convert.from.fp16.f32(
   // CHECK: fadd
   // CHECK: call i16 @llvm.convert.to.fp16.f32(
-  h0 += (__fp16)1.0;
+  h0 += (__fp16)1.0f;
   // CHECK: call float @llvm.convert.from.fp16.f32(
   // CHECK: fadd
   // CHECK: call i16 @llvm.convert.to.fp16.f32(
   h0 += f2;
 
   // CHECK: call float @llvm.convert.from.fp16.f32(
   // CHECK: call float @llvm.convert.from.fp16.f32(
   // CHECK: fsub
void foo(void) {
   // CHECK: call float @llvm.convert.from.fp16.f32(
   // CHECK: fdiv
   // CHECK: call i16 @llvm.convert.to.fp16.f32(
   h0 /= (__fp16)1.0;
   // CHECK: call float @llvm.convert.from.fp16.f32(
   // CHECK: fdiv
   // CHECK: call i16 @llvm.convert.to.fp16.f32(
   h0 /= f2;
+
+  // Check conversions to/from double
+  // CHECK: call i16 @llvm.convert.to.fp16.f64(
+  h0 = d0;
+
+  // CHECK: [[MID:%.*]] = fptrunc double {{%.*}} to float
+  // CHECK: call i16 @llvm.convert.to.fp16.f32(float [[MID]])
+  h0 = (float)d0;
+
+  // CHECK: call double @llvm.convert.from.fp16.f64(
+  d0 = h0;
+
+  // CHECK: [[MID:%.*]] = call float @llvm.convert.from.fp16.f32(
+  // CHECK: fpext float [[MID]] to double
+  d0 = (float)h0;
 }