[AArch64] added FP16 vcvth intrinsic support

[LukeGeeson]

Summary

Change-Id: I0df845749c7689dfc99150ba7c19c7d0dadbd705

[lebedev.ri]

test/CodeGen/AArch64/fp16_intrinsic_scalar_2op.ll

131–200 ↗

(On Diff #144703)

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 ↗

(On Diff #144703)

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

1 ↗

(On Diff #144710)

Please remove this line.

14 ↗

(On Diff #144710)

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.

146 ↗

(On Diff #144710)

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

147 ↗

(On Diff #144710)

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 ↗

(On Diff #144703)

missed this, will add thanks

[SjoerdMeijer]

test/CodeGen/AArch64/fp16_intrinsic_scalar_2op.ll

156 ↗

(On Diff #144725)

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

7793

Do we use OpNode?

7805

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

lib/Target/AArch64/AArch64InstrInfo.td

4869

Do we need to pass the intrinsics opnodes?

[LukeGeeson]

• [AArch64] removed OpNode+intrinsic template params

[LukeGeeson]

lib/Target/AArch64/AArch64InstrFormats.td

7793

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

5982

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

7804

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

7822

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

5982

Nit: trailing whitespace

7793

Nit: unnecessary new line.

7816

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.

7834

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 SIMDFPScalarRShift<bit U, bits<5> opc, string asm> {
 
   let Predicates = [HasNEON, HasFullFP16] in {
-  def SHr : BaseSIMDScalarShift<U, opc, {0,0,1,?,?,?,?},
-                                FPR32, FPR16, vecshiftR32, asm, []> {
+  def HSr : BaseSIMDScalarShift<U, opc, {0,0,1,?,?,?,?},
+                                FPR16, FPR32, vecshiftR16, asm, []> {
     let Inst{19-16} = imm{3-0};
     let Inst{23-22} = 0b11;
   }
+  } // Predicates = [HasNEON, HasFullFP16]
+
+  let Predicates = [HasNEON, HasFullFP16] in {
+  def SHr : BaseSIMDScalarShift<U, opc, {?,?,?,?,?,?,?},
+                                FPR32, FPR16, vecshiftR32, asm, []> {
+    let Inst{19-16} = imm{3-0};
+  }
 
   } // Predicates = [HasNEON, HasFullFP16]
 
   def HDr : BaseSIMDScalarShift<U, opc, {?,?,?,?,?,?,?},
-                                FPR16, FPR64, vecshiftR16, asm, []> {
+                                FPR16, FPR64, vecshiftR32, asm, []> {
     let Inst{21-16} = imm{5-0};
     let Inst{23-22} = 0b11;
   }

   }
 
   def d : BaseSIMDScalarShift<U, opc, {1,?,?,?,?,?,?},
-                              FPR64, FPR64, vecshiftR16, asm, []> {
+                              FPR64, FPR64, vecshiftR64, 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, []> {
+                              FPR16, FPR16, vecshiftR16, asm, []> {
     let Inst{19-16} = imm{3-0};
   }
   } // Predicates = [HasNEON, HasFullFP16]

lib/Target/AArch64/AArch64InstrInfo.td

                                             vecshiftR64:$imm)),
           (FCVTZUd FPR64:$Rn, vecshiftR64:$imm)>;
 def : Pat<(f16 (int_aarch64_neon_vcvtfxs2fp (i64 FPR64:$Rn), vecshiftR16:$imm)),
-          (FCVTZSHDr (i64 FPR64:$Rn), vecshiftR16:$imm)>;
+          (FCVTZSHDr (i64 FPR64:$Rn), vecshiftR32:$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<(f16 (int_aarch64_neon_vcvtfxu2fp FPR32:$Rn, vecshiftR16:$imm)),
+          (UCVTFHSr 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_vcvtfxu2fp (i64 FPR64:$Rn), vecshiftR64:$imm)),

                                             vecshiftR64:$imm)),
           (SCVTFd FPR64:$Rn, vecshiftR64:$imm)>;
 def : Pat<(f16 (int_aarch64_neon_vcvtfxs2fp (i32 FPR32:$Rn), vecshiftR16:$imm)),
-          (SCVTFh FPR32:$Rn, vecshiftR16:$imm)>;
+          (SCVTFHSr FPR32:$Rn, vecshiftR16:$imm)>;
 def : Pat<(f16 (int_aarch64_neon_vcvtfxs2fp FPR32:$Rn, vecshiftR16:$imm)),
-          (SCVTFh FPR32:$Rn, vecshiftR16:$imm)>;
+          (SCVTFHSr 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),