[PATCH] D15015: [AArch64] Add ARMv8.2-A FP16 vector instructions

[Ahmed Bougacha via llvm-commits]

ab added a subscriber: ab.
ab added a comment.

In general, no objections.

What do you think of introducing more base classes for the H and non-H variants? For instance, for BaseSIMDThreeSameVector, you could have something like BaseSIMDThreeSameVectorH which just overrides (or passes up, if we also added a BaseSIMDThreeSameVectorSD) the correct bit.  That could help avoid a lot of the noise, and keep the descriptions a little cleaner, I think.

Another thing is, many of the patterns are in standalone multiclasses, and would need f16 variants as well. I'll submit a patch if you don't beat me to it!

Also, you might want to split out more of the scalar instructions.


================
Comment at: lib/Target/AArch64/AArch64InstrFormats.td:4592
@@ -4567,3 +4591,3 @@
 let mayLoad = 0, mayStore = 0, hasSideEffects = 0 in
-class BaseSIMDTwoSameVector<bit Q, bit U, bits<2> size, bits<5> opcode,
+class BaseSIMDTwoSameVector<bit Q, bit U, bits<2> size, bits<5> opcode, bits<2> size2,
                         RegisterOperand regtype, string asm, string dstkind,
----------------
80-col? for Tied as well

Another really minor nit: what about ordering them like so: "size, size2, opcode"?

================
Comment at: lib/Target/AArch64/AArch64InstrFormats.td:4981-4984
@@ -4929,4 +4980,6 @@
+class BaseSIMDCmpTwoVector<bit Q, bit U, bits<2> size, bits<2> size2,
+                           bits<5> opcode,
                            RegisterOperand regtype,
                            string asm, string kind, string zero,
                            ValueType dty, ValueType sty, SDNode OpNode>
   : I<(outs regtype:$Rd), (ins regtype:$Rn), asm,
----------------
reflow?

================
Comment at: lib/Target/AArch64/AArch64InstrFormats.td:5700
@@ -5625,3 +5699,3 @@
 
-multiclass SIMDThreeScalarSD<bit U, bit S, bits<5> opc, string asm,
+multiclass SIMDThreeScalarSD<bit U, bit S, bits<3> opc, string asm,
                              SDPatternOperator OpNode = null_frag> {
----------------
SIMDThreeScalarSD -> SIMDThreeScalarHSD?
Or, as we do elsewhere -better IMO-, SIMDFPThreeScalar?

================
Comment at: lib/Target/AArch64/AArch64InstrFormats.td:5883
@@ +5882,3 @@
+  let Predicates = [HasNEON, HasFullFP16] in {
+  def : InstAlias<asm # " $Rd, $Rn, #0",
+                  (!cast<Instruction>(NAME # v1i16rz) FPR16:$Rd, FPR16:$Rn), 0>;
----------------
\t before operands?

================
Comment at: lib/Target/AArch64/AArch64InstrFormats.td:6053
@@ -5948,3 +6052,3 @@
 
 multiclass SIMDAcrossLanesS<bits<5> opcode, bit sz1, string asm,
                             Intrinsic intOp> {
----------------
SIMDAcrossLanesS -> HS ?

================
Comment at: lib/Target/AArch64/AArch64InstrFormats.td:6943-6945
@@ +6942,5 @@
+  def v8i16_indexed : BaseSIMDIndexedTied<1, U, 0, 0b00, opc,
+                                      V128, V128,
+                                      V128_lo, VectorIndexH,
+                                      asm, ".8h", ".8h", ".8h", ".h", []> {
+    bits<3> idx;
----------------
alignment?


Repository:
  rL LLVM

http://reviews.llvm.org/D15015