[ARM] FP16 VSEL codegen follow up

[SjoerdMeijer]

Summary

This is a follow up of rL327695, to instruction select
more variants of VSELGT and VSELGE, for which it is necessary
to custom lower SELECT.

More work is required in this area, which will be addressed soon:

• more variants need to be regression tested, but this depends on the next point.
• LowerConstantFP need to be adjusted for fp16 values.

[olista01]

lib/Target/ARM/ARMISelLowering.cpp

4529–4531

Does this need to check Subtarget->hasFullFP16()?

test/CodeGen/ARM/fp16-instructions.ll

740

What code actually gets emitted here? If it is correct but sub-optimal we should be testing it, otherwise this patch shouldn't be committed util the generated code is correct.

749

This only checks the HARDFP-FULLFP16 case, should it also be checking the others? Also, should we check the VCMP instruction, as well as the VSEL?

[SjoerdMeijer]

Thanks for checking!
I have guarded the lowering with HasFullFP16, and have temporarily removed the files, because at the moment they don't add anything. They just generate some vcmpe.f16, which we already tests. I will put these tests back when LowerConstantFP has been fixed and these cases also lower to VSELs.

[olista01]

This patch now changes 4 DAG nodes, but doesn't touch any tests for most of them. Are these changes NFC, and properly tested elsewhere? If not, this needs more tests.

[SjoerdMeijer]

Ah, yes, should have explained that a bit better. This is related to your earlier comment about:

Does this need to check Subtarget->hasFullFP16()?

Instead of doing the f16 and hasFullFP16() check inside function LowerSELECT_CC, I have guarded custom lowering of the Node with the hasFullFP16 check. While doing this, I spotted the custom lowering of SETCC, SELECT, SELECT_CC and BR_CC for f16, and I think it is more correct to guard these custom lowerings with hasFullFP16 as well; that's what we should be doing. Thus, this is indeed a non-functional change. And they should already be regression tested. BR_CC is for example added in D43508 and has tests, and file fp16-instructions.ll should test the others too.

[SjoerdMeijer]

I forgot to address one of your earlier comments, i.e. the one about adding more test cases.

[olista01]

test/CodeGen/ARM/fp16-instructions.ll

763

Why are we generating more code for this case than the others?

[SjoerdMeijer]

Why are we generating more code for this case than the others?

There was some interaction between using "undef" as an operand and this "unordered or equal" operator. I have changed the test case to make it more intuitive (and realistic). The generated code is identical to using float, except the .f16 and .f32 differences of course.

[olista01]

LGTM, thanks.

lib/Target/ARM/ARMISelLowering.cpp

ARMTargetLowering::ARMTargetLowering(const TargetMachine &TM,
   setOperationAction(ISD::INTRINSIC_WO_CHAIN, MVT::Other, Custom);
   setOperationAction(ISD::EH_SJLJ_SETJMP, MVT::i32, Custom);
   setOperationAction(ISD::EH_SJLJ_LONGJMP, MVT::Other, Custom);
   setOperationAction(ISD::EH_SJLJ_SETUP_DISPATCH, MVT::Other, Custom);
   if (Subtarget->useSjLjEH())
     setLibcallName(RTLIB::UNWIND_RESUME, "_Unwind_SjLj_Resume");
 
   setOperationAction(ISD::SETCC,     MVT::i32, Expand);
-  setOperationAction(ISD::SETCC,     MVT::f16, Expand);
   setOperationAction(ISD::SETCC,     MVT::f32, Expand);
   setOperationAction(ISD::SETCC,     MVT::f64, Expand);
   setOperationAction(ISD::SELECT,    MVT::i32, Custom);
   setOperationAction(ISD::SELECT,    MVT::f32, Custom);
   setOperationAction(ISD::SELECT,    MVT::f64, Custom);
-  setOperationAction(ISD::SELECT_CC, MVT::f16, Custom);
   setOperationAction(ISD::SELECT_CC, MVT::i32, Custom);
   setOperationAction(ISD::SELECT_CC, MVT::f32, Custom);
   setOperationAction(ISD::SELECT_CC, MVT::f64, Custom);
+  if (Subtarget->hasFullFP16()) {
+    setOperationAction(ISD::SETCC,     MVT::f16, Expand);
+    setOperationAction(ISD::SELECT,    MVT::f16, Custom);
+    setOperationAction(ISD::SELECT_CC, MVT::f16, Custom);
+  }
 
   // Thumb-1 cannot currently select ARMISD::SUBE.
   if (!Subtarget->isThumb1Only())
     setOperationAction(ISD::SETCCE, MVT::i32, Custom);
 
   setOperationAction(ISD::BRCOND,    MVT::Other, Custom);
   setOperationAction(ISD::BR_CC,     MVT::i32,   Custom);
-  setOperationAction(ISD::BR_CC,     MVT::f16,   Custom);
+  if (Subtarget->hasFullFP16())
+      setOperationAction(ISD::BR_CC, MVT::f16,   Custom);
   setOperationAction(ISD::BR_CC,     MVT::f32,   Custom);
   setOperationAction(ISD::BR_CC,     MVT::f64,   Custom);
   setOperationAction(ISD::BR_JT,     MVT::Other, Custom);
 
   // We don't support sin/cos/fmod/copysign/pow
   setOperationAction(ISD::FSIN,      MVT::f64, Expand);
   setOperationAction(ISD::FSIN,      MVT::f32, Expand);
   setOperationAction(ISD::FCOS,      MVT::f32, Expand);
SDValue ARMTargetLowering::LowerSELECT_CC(SDValue Op, SelectionDAG &DAG) const {
 
   ARMCC::CondCodes CondCode, CondCode2;
   bool InvalidOnQNaN;
   FPCCToARMCC(CC, CondCode, CondCode2, InvalidOnQNaN);
 
   // Normalize the fp compare. If RHS is zero we keep it there so we match
   // CMPFPw0 instead of CMPFP.
   if (Subtarget->hasFPARMv8() && !isFloatingPointZero(RHS) &&
-    (TrueVal.getValueType() == MVT::f32 || TrueVal.getValueType() == MVT::f64)) {
+     (TrueVal.getValueType() == MVT::f16 ||
+      TrueVal.getValueType() == MVT::f32 ||
+      TrueVal.getValueType() == MVT::f64)) {
     bool swpCmpOps = false;
     bool swpVselOps = false;
     checkVSELConstraints(CC, CondCode, swpCmpOps, swpVselOps);
 
     if (CondCode == ARMCC::GT || CondCode == ARMCC::GE ||
         CondCode == ARMCC::VS || CondCode == ARMCC::EQ) {
       if (swpCmpOps)
         std::swap(LHS, RHS);

test/CodeGen/ARM/fp16-instructions.ll

define half @select_cc1()  {
   %1 = fcmp nsz oeq half undef, 0xH0001
   %2 = select i1 %1, half 0xHC000, half 0xH0002
   ret half %2
 
 ; CHECK-LABEL:                 select_cc1:
 ; CHECK-HARDFP-FULLFP16:       vseleq.f16  s0, s{{.}}, s{{.}}
 }
 
+; FIXME: more tests need to be added for VSELGE and VSELGT.
+; That is, more combinations of immediate operands that can or can't
+; be encoded as an FP16 immediate need to be added here.
+;
 ; 36. VSELGE
-define half @select_cc2()  {
+define half @select_cc_ge1()  {
   %1 = fcmp nsz oge half undef, 0xH0001
   %2 = select i1 %1, half 0xHC000, half 0xH0002
   ret half %2
 
-; CHECK-LABEL:                 select_cc2:
+; CHECK-LABEL:                 select_cc_ge1:
+; CHECK-HARDFP-FULLFP16:       vselge.f16  s0, s{{.}}, s{{.}}
+}
+
+; FIXME: add fcmp ole, ult here.
+
+define half @select_cc_ge3()  {
+  %1 = fcmp nsz ugt half undef, 0xH0001
+  %2 = select i1 %1, half 0xHC000, half 0xH0002
+  ret half %2
+
+; CHECK-LABEL:                 select_cc_ge3:
 ; CHECK-HARDFP-FULLFP16:       vselge.f16  s0, s{{.}}, s{{.}}
 }
 
 ; 37. VSELGT
-define half @select_cc3()  {
+define half @select_cc_gt1()  {
   %1 = fcmp nsz ogt half undef, 0xH0001
   %2 = select i1 %1, half 0xHC000, half 0xH0002
   ret half %2
 
-; CHECK-LABEL:                 select_cc3:
+; CHECK-LABEL:                 select_cc_gt1:
 ; CHECK-HARDFP-FULLFP16:       vselgt.f16  s0, s{{.}}, s{{.}}
 }
 
+define half @select_cc_gt2()  {
+  %1 = fcmp nsz uge half undef, 0xH0001
+  %2 = select i1 %1, half 0xHC000, half 0xH0002
+  ret half %2
+
+; CHECK-LABEL:                 select_cc_gt2:
+; CHECK-HARDFP-FULLFP16:       vselgt.f16  s0, s{{.}}, s{{.}}
+}
+
+; FIXME: add fcmp ule, olt here.
+
 ; 38. VSELVS
 define half @select_cc4()  {
   %1 = fcmp nsz ueq half undef, 0xH0001
   %2 = select i1 %1, half 0xHC000, half 0xH0002
   ret half %2
 
 ; CHECK-LABEL:                 select_cc4:
 ; CHECK-HARDFP-FULLFP16:       vselvs.f16  s0, s{{.}}, s{{.}}
 }
 
 ; 39. VSQRT - TODO
 
 ; 40. VSUB
 define float @Sub(float %a.coerce, float %b.coerce) {
 entry:
   %0 = bitcast float %a.coerce to i32
   %tmp.0.extract.trunc = trunc i32 %0 to i16