[AArch64] added FP16 vcvth intrinsic support

[LukeGeeson]

Summary

Change-Id: I0df845749c7689dfc99150ba7c19c7d0dadbd705

[lebedev.ri]

test/CodeGen/AArch64/fp16_intrinsic_scalar_2op.ll

131–200

Is it intentional that there are no ; CHECK lines?

[SjoerdMeijer]

Hi Luke, thanks for fixing this, some first comments inlined.

lib/Target/AArch64/AArch64InstrFormats.td

5983

Remove comments

7793

nit: trailing whitespace?

7799

nit: don't think you break up the lines like this.

lib/Target/AArch64/AArch64InstrInfo.td

4880

Better not to introduce new line breaks here and also below.

test/CodeGen/AArch64/fp16_intrinsic_scalar_2op.ll

131

You should add CHECK lines for all these test cases (see examples above).

[LukeGeeson]

• [AArch64] rm'd linebreaks, added CHECKS to fp16 instrinsics

[SjoerdMeijer]

test/CodeGen/AArch64/fp16_intrinsic_scalar_2op.ll

13

Please don't modify the existing test cases. There is no need to check for the %bb.0 stuff as it doesn't add any value.

73

Please remove this line.

133

Please remove this line here and similar lines in the test cases below.

134

You should create a regexp for w8, and use it in the line below. Currently the test is very fragile, because if another register gets allocated this test starts failing.

[LukeGeeson]

• rm'd additional %bb lines, added regex for w8

[LukeGeeson]

give me one moment, syntax error missed

lib/Target/AArch64/AArch64InstrFormats.td

7799

Would you remove line 7803 too? looks better for separation

test/CodeGen/AArch64/fp16_intrinsic_scalar_2op.ll

131–200

missed this, will add thanks

[SjoerdMeijer]

test/CodeGen/AArch64/fp16_intrinsic_scalar_2op.ll

156

What is the supported range of constant 'n' for this intrinsic? If it is e.g. [1,16], I think it is best to test the minimum value 1, which is what we do here, but also the maximum value 16.
Same comment for the other intrinsics here.

[LukeGeeson]

• [AArch64] fixed syntax errors for FP16 intrinsics

[LukeGeeson]

• [AArch64] added tests for FP16 intrinsic ranges

[SjoerdMeijer]

lib/Target/AArch64/AArch64InstrFormats.td

7794

Do we use OpNode?

7807

Nit1: spaces are off: FPR16 should be aligned under U.
Nit2: space between ">{". Same for other rules below.

lib/Target/AArch64/AArch64InstrInfo.td

4869–4872

Do we need to pass the intrinsics opnodes?

[LukeGeeson]

• [AArch64] removed OpNode+intrinsic template params

[LukeGeeson]

lib/Target/AArch64/AArch64InstrFormats.td

7794

builds and tests fine without it - removed

[LukeGeeson]

• [AArch64] added spaces between [] and {

[SjoerdMeijer]

Thanks, looks good to me now. One more nit inlined, but no need for another review.

lib/Target/AArch64/AArch64InstrFormats.td

5983

No changes were made here, so can you keep the old formatting?

[SjoerdMeijer]

Sorry, I had one more look, see question inlined.

lib/Target/AArch64/AArch64InstrFormats.td

7807

Does this need to be vecshiftR32 and thus accept values [1,32]? If that's the case, we also need to update the tests.

7829–7830

We don't need to change this?

[LukeGeeson]

• [AArch64] modified fp16 instrinsics patterns

[LukeGeeson]

Local setup broken, please ignore

[LukeGeeson]

[Aarch64 reverting diff]

[LukeGeeson]

[AArch64] fixed FP16 intrinsic h pattern

[LukeGeeson]

• [AArch64] added 32 case for FP16 instrinsic case, fixed remarks

[LukeGeeson]

• [AArch64] fixed 32 case for FP16 s61 f16 test, 2op tests pass

[SjoerdMeijer]

lib/Target/AArch64/AArch64InstrFormats.td

5983

Nit: trailing whitespace

7794

Nit: unnecessary new line.

7820

Do the HDr and DHr patterns need to be guarded by predicates [HasNEON, HasFullFP16]? Can you check if they are all predicates are correctly set here? Also looks like we can simply things here a bit: merge all patterns with the same neon and fullfp16 predicates in one block.

7835

To keep the changes minimal, can you please move the "def h" back up where it was?

[LukeGeeson]

• [AArch64] moved FP16 h pattern, refactored FP16 intrinsics into block
• [AArch64] moved FP16 h pattern, refactored FP16 intrinsics into block

[LukeGeeson]

• [AArch64] removed whitespace for consistency

[SjoerdMeijer]

Thanks, I think this looks OK.

lib/Target/AArch64/AArch64InstrFormats.td

multiclass SIMDThreeScalarFPCmp<bit U, bit S, bits<3> opc, string asm,
                                 SDPatternOperator OpNode = null_frag> {
   let mayLoad = 0, mayStore = 0, hasSideEffects = 0 in {
     def #NAME#64 : BaseSIMDThreeScalar<U, {S,0b11}, {0b11,opc}, FPR64, asm,
       [(set (i64 FPR64:$Rd), (OpNode (f64 FPR64:$Rn), (f64 FPR64:$Rm)))]>;
     def #NAME#32 : BaseSIMDThreeScalar<U, {S,0b01}, {0b11,opc}, FPR32, asm,
       [(set (i32 FPR32:$Rd), (OpNode (f32 FPR32:$Rn), (f32 FPR32:$Rm)))]>;
     let Predicates = [HasNEON, HasFullFP16] in {
     def #NAME#16 : BaseSIMDThreeScalar<U, {S,0b10}, {0b00,opc}, FPR16, asm,
-      []>;
+      []>; //[(set (f16 FPR16:$Rd), (OpNode (f16 FPR16:$Rn), (f16 FPR16:$Rm)))]>;
     } // Predicates = [HasNEON, HasFullFP16]
   }
 
   def : Pat<(v1i64 (OpNode (v1f64 FPR64:$Rn), (v1f64 FPR64:$Rm))),
             (!cast<Instruction>(NAME # "64") FPR64:$Rn, FPR64:$Rm)>;
 }
 
 class BaseSIMDThreeScalarMixed<bit U, bits<2> size, bits<5> opcode,
class BaseSIMDScalarShiftTied<bit U, bits<5> opc, bits<7> fixed_imm,
   let Inst{22-16} = fixed_imm;
   let Inst{15-11} = opc;
   let Inst{10}    = 1;
   let Inst{9-5} = Rn;
   let Inst{4-0} = Rd;
 }
 
 
-multiclass SIMDFPScalarRShift<bit U, bits<5> opc, string asm> {
+multiclass SIMDFPScalarRShift<bit U, bits<5> opc, string asm, 
+                              SDPatternOperator OpNode=null_frag> { 
+  
   let Predicates = [HasNEON, HasFullFP16] in {
-  def h : BaseSIMDScalarShift<U, opc, {0,0,1,?,?,?,?},
-                              FPR16, FPR16, vecshiftR16, asm, []> {
+  def SHr : BaseSIMDScalarShift<U, opc, {0,0,1,?,?,?,?},
+                              FPR32, FPR16, vecshiftR32, asm, 
+          []>{
     let Inst{19-16} = imm{3-0};
+    let Inst{23-22} = 0b11;
   }
+
   } // Predicates = [HasNEON, HasFullFP16]
+ 
+  def HDr : BaseSIMDScalarShift<U, opc, {?,?,?,?,?,?,?},
+                              FPR16, FPR64, vecshiftR16, asm,  
+          []>{
+    let Inst{21-16} = imm{5-0};
+    let Inst{23-22} = 0b11; 
+  }
+
+  
+  def DHr : BaseSIMDScalarShift<U, opc, {?,?,?,?,?,?,?},
+                              FPR64, FPR16, vecshiftR64, asm,  
+          []>{
+    let Inst{21-16} = imm{5-0};
+    let Inst{23-22} = 0b11; 
+    let Inst{31} = 1;  
+  }
+
   def s : BaseSIMDScalarShift<U, opc, {0,1,?,?,?,?,?},
-                              FPR32, FPR32, vecshiftR32, asm, []> {
+                              FPR32, FPR32, vecshiftR32, asm, 
+    []> {
     let Inst{20-16} = imm{4-0};
   }
 
   def d : BaseSIMDScalarShift<U, opc, {1,?,?,?,?,?,?},
-                              FPR64, FPR64, vecshiftR64, asm, []> {
+                              FPR64, FPR64, vecshiftR16, asm, 
+         []>{ 
     let Inst{21-16} = imm{5-0};
   }
+
+  let Predicates = [HasNEON, HasFullFP16] in {
+  def h : BaseSIMDScalarShift<U, opc, {0,0,1,?,?,?,?},
+                              FPR16, FPR32, vecshiftR16, asm, 
+          []>{
+    let Inst{19-16} = imm{3-0};
+  }
+  } // Predicates = [HasNEON, HasFullFP16]
+   
 }
 
 multiclass SIMDScalarRShiftD<bit U, bits<5> opc, string asm,
                              SDPatternOperator OpNode> {
   def d : BaseSIMDScalarShift<U, opc, {1,?,?,?,?,?,?},
                               FPR64, FPR64, vecshiftR64, asm,
   [(set (i64 FPR64:$Rd),
      (OpNode (i64 FPR64:$Rn), (i32 vecshiftR64:$imm)))]> {

lib/Target/AArch64/AArch64InstrInfo.td

 def : Pat<(int_aarch64_neon_sqdmulls_scalar (i32 FPR32:$Rn),
                                           (vector_extract (v4i32 V128:$Vm),
                                                            VectorIndexS:$idx)),
           (SQDMULLv1i64_indexed FPR32:$Rn, V128:$Vm, VectorIndexS:$idx)>;
 
 //----------------------------------------------------------------------------
 // AdvSIMD scalar shift instructions
 //----------------------------------------------------------------------------
-defm FCVTZS : SIMDFPScalarRShift<0, 0b11111, "fcvtzs">;
-defm FCVTZU : SIMDFPScalarRShift<1, 0b11111, "fcvtzu">;
-defm SCVTF  : SIMDFPScalarRShift<0, 0b11100, "scvtf">;
-defm UCVTF  : SIMDFPScalarRShift<1, 0b11100, "ucvtf">;
+defm FCVTZS : SIMDFPScalarRShift<0, 0b11111, "fcvtzs", int_aarch64_neon_vcvtfp2fxs>;
+defm FCVTZU : SIMDFPScalarRShift<1, 0b11111, "fcvtzu", int_aarch64_neon_vcvtfp2fxu>;
+defm SCVTF  : SIMDFPScalarRShift<0, 0b11100, "scvtf",  int_aarch64_neon_vcvtfxs2fp>;
+defm UCVTF  : SIMDFPScalarRShift<1, 0b11100, "ucvtf", int_aarch64_neon_vcvtfxu2fp>;
 // Codegen patterns for the above. We don't put these directly on the
 // instructions because TableGen's type inference can't handle the truth.
 // Having the same base pattern for fp <--> int totally freaks it out.
 def : Pat<(int_aarch64_neon_vcvtfp2fxs FPR32:$Rn, vecshiftR32:$imm),
           (FCVTZSs FPR32:$Rn, vecshiftR32:$imm)>;
 def : Pat<(int_aarch64_neon_vcvtfp2fxu FPR32:$Rn, vecshiftR32:$imm),
           (FCVTZUs FPR32:$Rn, vecshiftR32:$imm)>;
+
 def : Pat<(i64 (int_aarch64_neon_vcvtfp2fxs (f64 FPR64:$Rn), vecshiftR64:$imm)),
           (FCVTZSd FPR64:$Rn, vecshiftR64:$imm)>;
 def : Pat<(i64 (int_aarch64_neon_vcvtfp2fxu (f64 FPR64:$Rn), vecshiftR64:$imm)),
           (FCVTZUd FPR64:$Rn, vecshiftR64:$imm)>;
+
 def : Pat<(v1i64 (int_aarch64_neon_vcvtfp2fxs (v1f64 FPR64:$Rn),
                                             vecshiftR64:$imm)),
           (FCVTZSd FPR64:$Rn, vecshiftR64:$imm)>;
 def : Pat<(v1i64 (int_aarch64_neon_vcvtfp2fxu (v1f64 FPR64:$Rn),
                                             vecshiftR64:$imm)),
           (FCVTZUd FPR64:$Rn, vecshiftR64:$imm)>;
-def : Pat<(int_aarch64_neon_vcvtfxs2fp FPR32:$Rn, vecshiftR32:$imm),
-          (SCVTFs FPR32:$Rn, vecshiftR32:$imm)>;
+def : Pat<(f16 (int_aarch64_neon_vcvtfxs2fp (i64 FPR64:$Rn), vecshiftR16:$imm)),
+          (FCVTZSHDr (i64 FPR64:$Rn), vecshiftR16:$imm)>;
+def : Pat<(i32 (int_aarch64_neon_vcvtfp2fxu FPR16:$Rn, vecshiftR32:$imm)),
+          (FCVTZUSHr FPR16:$Rn, vecshiftR32:$imm)>;
+def : Pat<(i32 (int_aarch64_neon_vcvtfp2fxs FPR16:$Rn, vecshiftR32:$imm)),
+          (FCVTZSSHr FPR16:$Rn, vecshiftR32:$imm)>;
+def : Pat<(i64 (int_aarch64_neon_vcvtfp2fxs (f16 FPR16:$Rn), vecshiftR64:$imm)),
+          (FCVTZSDHr (f16 FPR16:$Rn), vecshiftR64:$imm)>;
+def : Pat<(f16 (int_aarch64_neon_vcvtfxu2fp FPR32:$Rn, vecshiftR16:$imm)),       
+          (UCVTFh FPR32:$Rn, vecshiftR16:$imm)>;
 def : Pat<(int_aarch64_neon_vcvtfxu2fp FPR32:$Rn, vecshiftR32:$imm),
           (UCVTFs FPR32:$Rn, vecshiftR32:$imm)>;
-def : Pat<(f64 (int_aarch64_neon_vcvtfxs2fp (i64 FPR64:$Rn), vecshiftR64:$imm)),
-          (SCVTFd FPR64:$Rn, vecshiftR64:$imm)>;
 def : Pat<(f64 (int_aarch64_neon_vcvtfxu2fp (i64 FPR64:$Rn), vecshiftR64:$imm)),
           (UCVTFd FPR64:$Rn, vecshiftR64:$imm)>;
 def : Pat<(v1f64 (int_aarch64_neon_vcvtfxs2fp (v1i64 FPR64:$Rn),
                                             vecshiftR64:$imm)),
           (SCVTFd FPR64:$Rn, vecshiftR64:$imm)>;
+def : Pat<(f16 (int_aarch64_neon_vcvtfxs2fp (i32 FPR32:$Rn), vecshiftR16:$imm)),
+          (SCVTFh FPR32:$Rn, vecshiftR16:$imm)>;
+def : Pat<(f16 (int_aarch64_neon_vcvtfxs2fp FPR32:$Rn, vecshiftR16:$imm)),
+          (SCVTFh FPR32:$Rn, vecshiftR16:$imm)>;
+def : Pat<(f64 (int_aarch64_neon_vcvtfxs2fp (i64 FPR64:$Rn), vecshiftR64:$imm)),
+          (SCVTFd FPR64:$Rn, vecshiftR64:$imm)>;
 def : Pat<(v1f64 (int_aarch64_neon_vcvtfxu2fp (v1i64 FPR64:$Rn),
                                             vecshiftR64:$imm)),
           (UCVTFd FPR64:$Rn, vecshiftR64:$imm)>;
 
 defm SHL      : SIMDScalarLShiftD<   0, 0b01010, "shl", AArch64vshl>;
 defm SLI      : SIMDScalarLShiftDTied<1, 0b01010, "sli">;
 defm SQRSHRN  : SIMDScalarRShiftBHS< 0, 0b10011, "sqrshrn",
                                      int_aarch64_neon_sqrshrn>;

test/CodeGen/AArch64/fp16_intrinsic_scalar_2op.ll

 ; RUN: llc < %s -mtriple=aarch64-eabi -mattr=+v8.2a,+fullfp16  | FileCheck %s
 
 declare half @llvm.aarch64.sisd.fabd.f16(half, half)
 declare half @llvm.aarch64.neon.fmax.f16(half, half)
 declare half @llvm.aarch64.neon.fmin.f16(half, half)
 declare half @llvm.aarch64.neon.frsqrts.f16(half, half)
 declare half @llvm.aarch64.neon.frecps.f16(half, half)
 declare half @llvm.aarch64.neon.fmulx.f16(half, half)
 declare half @llvm.fabs.f16(half)
 
 define dso_local half @t_vabdh_f16(half %a, half %b) {
 ; CHECK-LABEL: t_vabdh_f16:
 ; CHECK:         fabd h0, h0, h1
 ; CHECK-NEXT:    ret
 entry:
   %vabdh_f16 = tail call half @llvm.aarch64.sisd.fabd.f16(half %a, half %b)
   ret half %vabdh_f16
 }
 
 define dso_local half @t_vabdh_f16_from_fsub_fabs(half %a, half %b) {
 ; CHECK-LABEL: t_vabdh_f16_from_fsub_fabs:

 ; CHECK:         fcmp h0, h1
 ; CHECK-NEXT:    csetm w0, ls
 ; CHECK-NEXT:    ret
 entry:
   %0 = fcmp ole half %a, %b
   %vcmpd = sext i1 %0 to i16
   ret i16 %vcmpd
 }
 
 define dso_local i16 @t_vclth_f16(half %a, half %b) {
 ; CHECK-LABEL: t_vclth_f16:
 ; CHECK:         fcmp h0, h1
 ; CHECK-NEXT:    csetm w0, mi
 ; CHECK-NEXT:    ret
 entry:
   %0 = fcmp olt half %a, %b
   %vcmpd = sext i1 %0 to i16

 define dso_local half @t_vrsqrtsh_f16(half %a, half %b) {
 ; CHECK-LABEL: t_vrsqrtsh_f16:
 ; CHECK:         frsqrts h0, h0, h1
 ; CHECK-NEXT:    ret
 entry:
   %vrsqrtsh_f16 = tail call half @llvm.aarch64.neon.frsqrts.f16(half %a, half %b)
   ret half %vrsqrtsh_f16
 }
+
+
+define dso_local half @test_vcvth_n_f16_s16(i16 %a) local_unnamed_addr #0 {
+entry:
+  %sext = sext i16 %a to i32
+  %fcvth_n = tail call half @llvm.aarch64.neon.vcvtfxs2fp.f16.i32(i32 %sext, i32 1)
+  ret half %fcvth_n
+}
+
+declare half @llvm.aarch64.neon.vcvtfxs2fp.f16.i32(i32, i32) #1
+declare half @llvm.aarch64.neon.vcvtfxs2fp.f16.i64(i64, i32) #1
+declare i32 @llvm.aarch64.neon.vcvtfp2fxs.i32.f16(half, i32) #1
+declare i64 @llvm.aarch64.neon.vcvtfp2fxs.i64.f16(half, i32) #1
+declare half @llvm.aarch64.neon.vcvtfxu2fp.f16.i32(i32, i32) #1
+declare i32 @llvm.aarch64.neon.vcvtfp2fxu.i32.f16(half, i32) #1
+
+define dso_local half @test_vcvth_n_f16_s32(i32 %a) local_unnamed_addr #0 {
+entry:
+  %vcvth_n_f16_s32 = tail call half @llvm.aarch64.neon.vcvtfxs2fp.f16.i32(i32 %a, i32 1)
+  ret half %vcvth_n_f16_s32
+}
+
+define dso_local half @test_vcvth_n_f16_s64(i64 %a) local_unnamed_addr #0 {
+entry:
+  %vcvth_n_f16_s64 = tail call half @llvm.aarch64.neon.vcvtfxs2fp.f16.i64(i64 %a, i32 1)
+  ret half %vcvth_n_f16_s64
+}
+
+define dso_local i16 @test_vcvth_n_s16_f16(half %a) local_unnamed_addr #0 {
+entry:
+  %fcvth_n = tail call i32 @llvm.aarch64.neon.vcvtfp2fxs.i32.f16(half %a, i32 1)
+  %0 = trunc i32 %fcvth_n to i16
+  ret i16 %0
+}
+
+define dso_local i32 @test_vcvth_n_s32_f16(half %a) local_unnamed_addr #0 {
+entry:
+  %vcvth_n_s32_f16 = tail call i32 @llvm.aarch64.neon.vcvtfp2fxs.i32.f16(half %a, i32 1)
+  ret i32 %vcvth_n_s32_f16
+}
+
+define dso_local i64 @test_vcvth_n_s64_f16(half %a) local_unnamed_addr #0 {
+entry:
+  %vcvth_n_s64_f16 = tail call i64 @llvm.aarch64.neon.vcvtfp2fxs.i64.f16(half %a, i32 1)
+  ret i64 %vcvth_n_s64_f16
+}
+
+define dso_local half @test_vcvth_n_f16_u16(i16 %a) local_unnamed_addr #0 {
+entry:
+  %0 = zext i16 %a to i32
+  %fcvth_n = tail call half @llvm.aarch64.neon.vcvtfxu2fp.f16.i32(i32 %0, i32 1)
+  ret half %fcvth_n
+}
+
+define dso_local half @test_vcvth_n_f16_u32(i32 %a) local_unnamed_addr #0 {
+entry:
+  %vcvth_n_f16_u32 = tail call half @llvm.aarch64.neon.vcvtfxu2fp.f16.i32(i32 %a, i32 1)
+  ret half %vcvth_n_f16_u32
+}
+
+define dso_local i16 @test_vcvth_n_u16_f16(half %a) local_unnamed_addr #0 {
+entry:
+  %fcvth_n = tail call i32 @llvm.aarch64.neon.vcvtfp2fxu.i32.f16(half %a, i32 1)
+  %0 = trunc i32 %fcvth_n to i16
+  ret i16 %0
+}
+
+define dso_local i32 @test_vcvth_n_u32_f16(half %a) local_unnamed_addr #0 {
+entry:
+  %vcvth_n_u32_f16 = tail call i32 @llvm.aarch64.neon.vcvtfp2fxu.i32.f16(half %a, i32 1)
+  ret i32 %vcvth_n_u32_f16
+}