[PATCH] D46311: [AArch64] added FP16 vcvth intrinsic support

[Sjoerd Meijer via Phabricator via llvm-commits]

SjoerdMeijer added a comment.

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



================
Comment at: lib/Target/AArch64/AArch64InstrFormats.td:5983
     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]
----------------
Remove comments


================
Comment at: lib/Target/AArch64/AArch64InstrFormats.td:7793
 
-multiclass SIMDFPScalarRShift<bit U, bits<5> opc, string asm> {
+multiclass SIMDFPScalarRShift<bit U, bits<5> opc, string asm, 
+                              SDPatternOperator OpNode=null_frag> { 
----------------
nit: trailing whitespace?


================
Comment at: lib/Target/AArch64/AArch64InstrFormats.td:7799
+                              FPR32, FPR16, vecshiftR32, asm, 
+          []>{
     let Inst{19-16} = imm{3-0};
----------------
nit: don't think you break up the lines like this.


================
Comment at: lib/Target/AArch64/AArch64InstrInfo.td:4880
           (FCVTZUs FPR32:$Rn, vecshiftR32:$imm)>;
+
 def : Pat<(i64 (int_aarch64_neon_vcvtfp2fxs (f64 FPR64:$Rn), vecshiftR64:$imm)),
----------------
Better not to introduce new line breaks here and also below.


================
Comment at: test/CodeGen/AArch64/fp16_intrinsic_scalar_2op.ll:131
+
+define dso_local half @test_vcvth_n_f16_s16(i16 %a) local_unnamed_addr #0 {
+entry:
----------------
lebedev.ri wrote:
> Is it intentional that there are no `; CHECK` lines?
You should add CHECK lines for all these test cases (see examples above).


Repository:
  rL LLVM

https://reviews.llvm.org/D46311