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

[Sjoerd Meijer via Phabricator via llvm-commits]

SjoerdMeijer added inline comments.


================
Comment at: lib/Target/AArch64/AArch64InstrFormats.td:5982
     let Predicates = [HasNEON, HasFullFP16] in {
-    def #NAME#16 : BaseSIMDThreeScalar<U, {S,0b10}, {0b00,opc}, FPR16, asm,
-      []>;
+    def #NAME#16 : BaseSIMDThreeScalar<U, {S,0b10}, {0b00,opc}, FPR16, asm, []>; 
     } // Predicates = [HasNEON, HasFullFP16]
----------------
Nit: trailing whitespace


================
Comment at: lib/Target/AArch64/AArch64InstrFormats.td:7793
 multiclass SIMDFPScalarRShift<bit U, bits<5> opc, string asm> {
+
   let Predicates = [HasNEON, HasFullFP16] in {
----------------
Nit: unnecessary new line.


================
Comment at: lib/Target/AArch64/AArch64InstrFormats.td:7816
+
+  def DHr : BaseSIMDScalarShift<U, opc, {?,?,?,?,?,?,?},
+                                FPR64, FPR16, vecshiftR64, asm, []> {
----------------
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.


================
Comment at: lib/Target/AArch64/AArch64InstrFormats.td:7834
+  let Predicates = [HasNEON, HasFullFP16] in {
+  def h : BaseSIMDScalarShift<U, opc, {0,0,1,?,?,?,?},
+                              FPR16, FPR16, vecshiftR16, asm, []> {
----------------
To keep the changes minimal, can you please move the "def h" back up where it was?


Repository:
  rL LLVM

https://reviews.llvm.org/D46311