Always promote f16

[olista01]

Summary

Bug: https://llvm.org/bugs/show_bug.cgi?id=24549

• Softening support of f16 was never finished, really. Always promote f16 to f32. If f32 is not legal, rely on f32 legalization after promotion.
• Update SoftenFloatRes_FP_EXTEND to look for a promoted operand if the operand's type action is to promote.
• Add the test case from the linked bug.

Ideally, we should test all the cases in
test/CodeGen/ARM/fp16-promote.ll with -vfp2. Doing that needs extra
work:

• some functions in this test don't compile
• all the functions need a new set of CHECKs for when both f32 and f16 are illegal.

[pirama]

Created https://llvm.org/bugs/show_bug.cgi?id=24814 to track testing of fp16-promote.ll with -vfp2.

[pirama]

Ping...

[ab]

lib/CodeGen/SelectionDAG/LegalizeFloatTypes.cpp

424–425

IIUC, this means we'll do something like:

(f32 (fpext (f16 ...)))
(i32 (fpext (f32 <promoted f16 ...>)))
(i32 (__gnu_h2f_ieee (f32 ...)))
(i32 (__gnu_h2f_ieee (i32 ...)))

even though __gnu_h2f_ieee expects uint16_t. On ARM uint16_t will get promoted to i32, but now we're interpreting a full f32-softened-to-i32 as an i16-promoted-to-i32, no?

Doesn't this mean it's possible to get stuff like:

(i32 (__gnu_h2f_ieee
  (i32 (__gnu_h2f_ieee
    (i32 (load zext from i16 ...))
  ))
))

by legalizing:

(f32 (fpext (f16 (load ...))))

?

[pirama]

lib/CodeGen/SelectionDAG/LegalizeFloatTypes.cpp

424–425

You are right. I completely missed testing this. Also, Op isn't legalized yet at this point. I'll fix this and upload a new revision.

[olista01]

Hi Pirama,

Are you still working on this? One of our internal tests suites is currently failing for fp16, and this looks like it should fix it (except for the FPEXT issue Ahmed spotted).

Oliver

[pirama]

Hi Oliver, I started on a different activity and completely forgot about this. Hopefully, I'll have some time late next week to look into this. Please feel free to take over this patch if you have the cycles to fix the FPEXT issue.

Thanks,
-Pirama

[olista01]

Updated to correctly handle FPEXT, and get test/CodeGen/ARM/fp16-promote.ll passing. The changes in SelectionDAGBuilder.cpp were required for a few of the test cases, notably those with half-precision arguments (which the clang frontend doesn't currently emit, at least for ARM), and phi nodes.

[ab]

I think this is fine, modulo a couple nits.

Pirama: any comments?

lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp

201 ↗

(On Diff #38824)

&& bitsLT ?

getNode() is pretty involved, and here we'd end up calling it to insert a no-op truncate for every softened f32/f64, right?

test/CodeGen/ARM/fp16-promote.ll

153–156 ↗

(On Diff #38824)

-LIBCALL/-ALL ?

885–886 ↗

(On Diff #38824)

Did we start emitting anything else? If not, I'd keep the -NEXT and .fnstart.

[pirama]

LGTM, after ab's changes. Thanks for taking over this patch and also updating the test. Can you also clean up the commit message, now that you updated the test? (Or did you already update it and it is just not visible in Phabricator?)

[olista01]

Thanks, committed revision 252459 with the suggested changes.

test/CodeGen/ARM/fp16-promote.ll

885–886 ↗

(On Diff #38824)

Yes, we emit a mov here because the soft-float calling convention is different. I'll update the test to make this explicit.

lib/CodeGen/SelectionDAG/LegalizeFloatTypes.cpp

if (Op.getValueType() == MVT::f16 && N->getValueType(0) != MVT::f32) {
     Op = DAG.getNode(ISD::FP_EXTEND, SDLoc(N), MVT::f32, Op);
     if (getTypeAction(MVT::f32) == TargetLowering::TypeSoftenFloat)
       SoftenFloatResult(Op.getNode(), 0);
   }
 
   RTLIB::Libcall LC = RTLIB::getFPEXT(Op.getValueType(), N->getValueType(0));
   if (getTypeAction(Op.getValueType()) == TargetLowering::TypeSoftenFloat)
     Op = GetSoftenedFloat(Op);
+  else if (getTypeAction(Op.getValueType()) == TargetLowering::TypePromoteFloat)
+    Op = GetPromotedFloat(Op);
   assert(LC != RTLIB::UNKNOWN_LIBCALL && "Unsupported FP_EXTEND!");
   return TLI.makeLibCall(DAG, LC, NVT, &Op, 1, false, SDLoc(N)).first;
 }
 
 // FIXME: Should we just use 'normal' FP_EXTEND / FP_TRUNC instead of special
 // nodes?
 SDValue DAGTypeLegalizer::SoftenFloatRes_FP16_TO_FP(SDNode *N) {
   EVT MidVT = TLI.getTypeToTransformTo(*DAG.getContext(), MVT::f32);

lib/CodeGen/TargetLoweringBase.cpp

void TargetLoweringBase::computeRegisterProperties(
   if (!isTypeLegal(MVT::f32)) {
     NumRegistersForVT[MVT::f32] = NumRegistersForVT[MVT::i32];
     RegisterTypeForVT[MVT::f32] = RegisterTypeForVT[MVT::i32];
     TransformToType[MVT::f32] = MVT::i32;
     ValueTypeActions.setTypeAction(MVT::f32, TypeSoftenFloat);
   }
 
   if (!isTypeLegal(MVT::f16)) {
-    // If the target has native f32 support, promote f16 operations to f32.  If
-    // f32 is not supported, generate soft float library calls.
-    if (isTypeLegal(MVT::f32)) {
-      NumRegistersForVT[MVT::f16] = NumRegistersForVT[MVT::f32];
-      RegisterTypeForVT[MVT::f16] = RegisterTypeForVT[MVT::f32];
-      TransformToType[MVT::f16] = MVT::f32;
-      ValueTypeActions.setTypeAction(MVT::f16, TypePromoteFloat);
-    } else {
-      NumRegistersForVT[MVT::f16] = NumRegistersForVT[MVT::i16];
-      RegisterTypeForVT[MVT::f16] = RegisterTypeForVT[MVT::i16];
-      TransformToType[MVT::f16] = MVT::i16;
-      ValueTypeActions.setTypeAction(MVT::f16, TypeSoftenFloat);
-    }
+    // Promote f16 operations to f32.  For targets where f32 is not legal, the
+    // promoted value would then undergo f32 legalization.
   }
 
   // Loop over all of the vector value types to see which need transformations.
   for (unsigned i = MVT::FIRST_VECTOR_VALUETYPE;
        i <= (unsigned)MVT::LAST_VECTOR_VALUETYPE; ++i) {
     MVT VT = (MVT::SimpleValueType) i;
     if (isTypeLegal(VT))
       continue;

test/CodeGen/ARM/fp16-no-fpu.ll

+; RUN: llc -asm-verbose=false -mattr=-vfp2 < %s | FileCheck %s
+
+target datalayout = "e-m:e-p:32:32-i64:64-v128:64:128-a:0:32-n32-S64"
+target triple = "armv7--none-eabi"
+
+; CHECK-LABEL: test_fadd:
+; CHECK: bl __gnu_h2f_ieee
+; CHECK: bl __gnu_h2f_ieee
+; CHECK: bl __aeabi_fadd
+; CHECK: bl __gnu_f2h_ieee
+define void @test_fadd(half* %p, half* %q) #0 {
+  %a = load half, half* %p, align 2
+  %b = load half, half* %q, align 2
+  %r = fadd half %a, %b
+  store half %r, half* %p
+  ret void
+}