[PATCH] D37145: [AArch64] allow v4f16 types when FullFP16 is supported

[Oliver Stannard via Phabricator via llvm-commits]

olista01 added inline comments.


================
Comment at: lib/Target/AArch64/AArch64ISelLowering.cpp:397
   // Expand all other v4f16 operations.
   // FIXME: We could generate better code by promoting some operations to
   // a pair of v4f32s
----------------
If there anything stopping us from doing this by just changing the operation action and promoted type here? We are already doing this for the simple arithmetic operations.


================
Comment at: test/CodeGen/AArch64/arm64-vfloatintrinsics.ll:17
+define %v4f16 @test_v4f16.sqrt(%v4f16 %a) {
+  ; CHECK-FP16: fsqrt.4h
+  %1 = call %v4f16 @llvm.sqrt.v4f16(%v4f16 %a)
----------------
We should also test how this gets lowered when we don't have FullFP16.


================
Comment at: test/CodeGen/AArch64/fp16-v4-instructions.ll:105
 define <4 x half> @d_to_h(<4 x double> %a) {
-; CHECK-LABEL: d_to_h:
-; CHECK-DAG: fcvt
-; CHECK-DAG: fcvt
-; CHECK-DAG: fcvt
-; CHECK-DAG: fcvt
-; CHECK-DAG: ins
-; CHECK-DAG: ins
-; CHECK-DAG: ins
-; CHECK-DAG: ins
+; CHECK-COMMON-LABEL: d_to_h:
+; CHECK-COMMON: mov
----------------
These two could be done with vector conversion instructions, rather than using the scalar conversions then rebuilding the vector. These instructions don't require FullFP16, so a separate patch would make most sense.


================
Comment at: test/CodeGen/AArch64/fp16-v4-instructions.ll:304
+; CHECK-CVT-LABEL: test_fcmp_une:
+; CHECK-CVT-DAG: fcvt
+; CHECK-CVT-DAG: fcvt
----------------
I think this could use FCVTL, like the arithmetic operations. This should be a separate patch though. (same for the rest of the comparison tests)


================
Comment at: test/CodeGen/AArch64/fp16-v4-instructions.ll:318
+; CHECK-FP16-LABEL: test_fcmp_une:
+; CHECK-FP16-NOT:   fcvt
+; CHECK-FP16:       ret
----------------
We should check that an FCMP is emitted here. (same for the rest of the comparison tests)


https://reviews.llvm.org/D37145