[PATCH] D62670: [ARM] Add MVE horizontal accumulation instructions.

[Oliver Stannard (Linaro) via Phabricator via llvm-commits]

ostannard added inline comments.


================
Comment at: llvm/lib/Target/ARM/ARMInstrMVE.td:367
+
+class t2VMINNM<string iname, string suffix, bit sz, bit bit_17,
+               list<dag> pattern=[]>
----------------
There seems to be a lot of repetition here, could some of these classes be combined? For example most bits of t2VMINNM, t2VMINV, t2VMAXNMV and t2VMAX are the same. Could we also use multiclasses to avoid repeating the list of types for each of the MIN and MAX versions of these instructions?

Also, is the a pattern to whether these names end in "V" which I've missed, or could they be made more consistent?


================
Comment at: llvm/lib/Target/ARM/ARMInstrMVE.td:382
+  let Inst{8-6} = 0b110;
+  let Inst{5} = 0b0; // undefined if 1
+  let Inst{3-1} = Qm{2-0};
----------------
What is this comment adding?


================
Comment at: llvm/lib/Target/ARM/ARMInstrMVE.td:487
+
+class t2VMLADAV<string iname, string suffix, bit sz, bit U,
+                bit A, bit X, bit bit_8, list<dag> pattern=[]>
----------------
Again, the t2VMLADAV and t2VMLSDAV classes share most of their bits, and could be merged.


================
Comment at: llvm/test/MC/ARM/mve-reductions.s:14
+
+# CHECK: vpte.i8 eq, q0, q0
+# CHECK-NOFP: vpte.i8 eq, q0, q0
----------------
We could avoid repeating all of these check lines by either adding a new check-prefix shared between the two runs, or moving the FP instructions to a separate file. As noted in previous reviews, we should also be checking the error message when the FP instructions are rejected.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D62670/new/

https://reviews.llvm.org/D62670