[PATCH] D62673: [ARM] Add MVE vector bit-operations (register inputs).

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

ostannard added inline comments.


================
Comment at: llvm/lib/Target/ARM/ARMInstrMVE.td:1306
+  let Inst{21-20} = 0b11;
+  let Inst{19-18} = size{1-0};
+  let Inst{17-16} = 0b00;
----------------
"size" is a 2-bit value, so "size{1-0}" can be simplified to just "size". (repeated in this file)


================
Comment at: llvm/lib/Target/ARM/AsmParser/ARMAsmParser.cpp:1947
 
+  template <int size>
+  bool isMVEVectorIndex() const {
----------------
Do we need a new function for this, rather than re-using the NEON ones above?


================
Comment at: llvm/lib/Target/ARM/AsmParser/ARMAsmParser.cpp:6679
+    // For vmov instructions, as mentioned earlier, we did not add the vector
+    // predication code, since these may contain operands that require
+    // special parsing.  So now we have to see if they require vector
----------------
Which operand require special parsing? This comment sounds like it's referring to operands with a custom ParserMethod listed in the tablegen description, but this patch doesn't add any of those.


================
Comment at: llvm/lib/Target/ARM/Disassembler/ARMDisassembler.cpp:6053
+
+  unsigned Imm = 0;
+  unsigned Beat = ((Val >> 2) & 1) | ((Val >> 3) & 2);
----------------
This could do with a comment explaining the two encodings we're converting between (especially the MCOperand one, as that's not documented elsewhere).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D62673