[ARM] Narrowing half-precision lowering to supported CCs

[plotfi]

Summary

I had noticed an assert in the ARM backend on builds of the Swift stdlib. I've since reduced the offending IR and reported my findings to the folks on D75169. I suspect that the changes in D75169 were meant mainly for the AAPCS calling conventions and indeed they do work fine with them, but with fastcc and swiftcc I noticed there was a crash.

I also noticed that the line that checks the calling convention in D75169 is only checking to see that there is a calling convention, not specifically checking that it is a supported calling convention. So this patch does just that, and includes a regression test too.

[pratlucas]

Hi @plotfi,

I believe simply limiting the calling conventions might be a bit tricky. At this point the method might get a calling convention id that does not reflect the one required for argument lowering (see ARMTargetLowering::getEffectiveCallingConv in ARMISelLowering.cpp).
One option to get around this would be to use that same method to determine the effective calling convention, but it might not be easy to check whether or not the function beeing handled is variadic.

Also, I did not expect to see a difference in the resulting parts' types between getCopyToParts and ARMTargetLowering::splitValueIntoRegisterParts, only in how the final type is achieved. I'm investigating this further to see what's missing.

[pratlucas]

Taking a further look to the differences in the DAG, the resulting type is actually the same.
Both result in an f32 value, but get to it in different ways as expected.

With ARMTargetLowering::splitValueIntoRegisterParts:

t22: f32 = bitcast t21
  t21: i32 = any_extend t20
    t20: i16 = bitcast t5
      t5: f16 = extract_vector_elt t3, Constant:i32<0>
        t3: v8f16,v8f16 = merge_values t2, t2
          t2: v8f16 = BUILD_VECTOR ConstantFP:f16<APFloat(0)>, ConstantFP:f16<APFloat(0)>, ConstantFP:f16<APFloat(0)>, ConstantFP:f16<APFloat(0)>, ConstantFP:f16<APFloat(0)>, ConstantFP:f
16<APFloat(0)>, ConstantFP:f16<APFloat(0)>, ConstantFP:f16<APFloat(0)>

Without ARMTargetLowering::splitValueIntoRegisterParts:

t20: f32 = fp_extend t5
  t5: f16 = extract_vector_elt t3, Constant:i32<0>
    t3: v8f16,v8f16 = merge_values t2, t2
      t2: v8f16 = BUILD_VECTOR ConstantFP:f16<APFloat(0)>, ConstantFP:f16<APFloat(0)>, ConstantFP:f16<APFloat(0)>, ConstantFP:f16<APFloat(0)>, ConstantFP:f16<APFloat(0)>, ConstantFP:f16<A
PFloat(0)>, ConstantFP:f16<APFloat(0)>, ConstantFP:f16<APFloat(0)>

I'm taking a deeper look into the assertion failure stack to figure out what's causing it.

[pratlucas]

It seems most of the argument lowering convertions get simplifyed on the regular case, when not using fastcc.
When using it, though, type legalization ends up trying to handle a i16 = bitcast ConstantFP:f16<APFloat(0)> node, turning it into an i32 <- f32 operation and getting lost while trying to create the new constant.

Avoiding the splitting should do the trick for fastcc, as it has no need to conform with AAPCS. I'm not sure about swiftcc, though, as it is expected to comply with AAPCS-VFP on ios platforms according to LLVM's language reference manual.

llvm/lib/Target/ARM/ARMISelLowering.cpp

4159 ↗

(On Diff #272940)

CallingConv::ARM_AAPCS_VFP and CallingConv::C would also need to be included here.

[plotfi]

In D82443#2111882, @pratlucas wrote:

It seems most of the argument lowering convertions get simplifyed on the regular case, when not using fastcc.
When using it, though, type legalization ends up trying to handle a i16 = bitcast ConstantFP:f16<APFloat(0)> node, turning it into an i32 <- f32 operation and getting lost while trying to create the new constant.

Avoiding the splitting should do the trick for fastcc, as it has no need to conform with AAPCS. I'm not sure about swiftcc, though, as it is expected to comply with AAPCS-VFP on ios platforms according to LLVM's language reference manual.

llvm/lib/Target/ARM/ARMISelLowering.cpp

4159 ↗

(On Diff #272940)

I believe I tried ARM_AAPCS_VFP and it asserted. Lemme confirm.

[plotfi]

In D82443#2111882, @pratlucas wrote:

It seems most of the argument lowering convertions get simplifyed on the regular case, when not using fastcc.
When using it, though, type legalization ends up trying to handle a i16 = bitcast ConstantFP:f16<APFloat(0)> node, turning it into an i32 <- f32 operation and getting lost while trying to create the new constant.

Avoiding the splitting should do the trick for fastcc, as it has no need to conform with AAPCS. I'm not sure about swiftcc, though, as it is expected to comply with AAPCS-VFP on ios platforms according to LLVM's language reference manual.

I gave the following a try, and it also asserts:

target datalayout = "e-m:o-p:32:32-Fi8-f64:32:64-v64:32:64-v128:32:128-a:0:32-n32-S32"
target triple = "thumbv7s-apple-ios7.0.0"

define arm_aapcs_vfpcc { <8 x half>, <8 x half> } @f() {
  ret { <8 x half>, <8 x half> } zeroinitializer
}

[pratlucas]

I've posted D82552 fixing the assertion failure on the type legalization's bitcast handling. Once it lands, it should be ok to include arm_aapcs_fvpcc and swiftcc on this patch.

[plotfi]

Updated diff to only handle fastcc

[plotfi]

@pratlucas I've updated this diff to only exclude fastcc. How does this look to you? Can I land this for now, as it does not interfere with your D82552 diff?

[pratlucas]

@plotfi This should not interfere D82552.
I believe you can land this, LGTM.

[plotfi]

Churning diff (no change) to see if Harbormaster will pass.

[plotfi]

Simplifying diff into a oneliner.

[plotfi]

D82552 appears to have also fixed fastcc, so I will only land the test case.

llvm/test/CodeGen/ARM/arm-half-promote.ll

 ; CHECK-NEXT: vmov.f32        s9, s16
 ; CHECK-NEXT: vmov.f32        s11, s16
 ; CHECK-NEXT: vmov.f32        s13, s16
 ; CHECK-NEXT: vmov.f32        s15, s16
 ; CHECK-NEXT: vpop    {d8}
 ; CHECK-NEXT: bx      lr
   ret { <8 x half>, <8 x half> } zeroinitializer
 }
+
+define fastcc { <8 x half>, <8 x half> } @f3() {
+; CHECK-LABEL: _f3
+; CHECK: vpush   {d8}
+; CHECK-NEXT: vmov.f64        d8, #5.000000e-01
+; CHECK-NEXT: vmov.i32        d8, #0x0
+; CHECK-NEXT: vmov.i32        d0, #0x0
+; CHECK-NEXT: vmov.i32        d1, #0x0
+; CHECK-NEXT: vmov.i32        d2, #0x0
+; CHECK-NEXT: vmov.i32        d3, #0x0
+; CHECK-NEXT: vmov.i32        d4, #0x0
+; CHECK-NEXT: vmov.i32        d5, #0x0
+; CHECK-NEXT: vmov.i32        d6, #0x0
+; CHECK-NEXT: vmov.i32        d7, #0x0
+; CHECK-NEXT: vmov.f32        s1, s16
+; CHECK-NEXT: vmov.f32        s3, s16
+; CHECK-NEXT: vmov.f32        s5, s16
+; CHECK-NEXT: vmov.f32        s7, s16
+; CHECK-NEXT: vmov.f32        s9, s16
+; CHECK-NEXT: vmov.f32        s11, s16
+; CHECK-NEXT: vmov.f32        s13, s16
+; CHECK-NEXT: vmov.f32        s15, s16
+; CHECK-NEXT: vpop    {d8}
+; CHECK-NEXT: bx      lr
+
+  ret { <8 x half>, <8 x half> } zeroinitializer
+}
+