[PATCH] D62680: [ARM] Add MVE vector load/store instructions.

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

ostannard added inline comments.


================
Comment at: llvm/lib/Target/ARM/ARMInstrMVE.td:161
+class taddrmode_imm7<int shift> : MemOperand {
+  let ParserMatchClass =
+   !cast<AsmOperandClass>("TMemImm7Shift"#shift#"OffsetAsmOperand");
----------------
I think this can be written like this, without the defs above:

  let ParserMatchClass = TMemImm7ShiftOffsetAsmOperand<shift>;


================
Comment at: llvm/lib/Target/ARM/ARMInstrMVE.td:3611
+
+// Trivial multiclass (only defining one thing), with the same API as
+// the previous one, for use when the memory size is one byte, so
----------------
Why is this needed, could we just define instances of MVE_VLDRSTR_rq directly below?


================
Comment at: llvm/lib/Target/ARM/AsmParser/ARMAsmParser.cpp:5812
+  else if (ShiftName == "uxtw" || ShiftName == "UXTW")
+    St= ARM_AM::uxtw;
   else
----------------
Missing space before equals


================
Comment at: llvm/lib/Target/ARM/AsmParser/ARMAsmParser.cpp:7487
+    if (Qd == Qm) {
+      return Error(Operands[3]->getStartLoc(),
+                   Twine("destination vector register and vector ") +
----------------
I don't see any tests for the QmIsPointer version of this error.


================
Comment at: llvm/lib/Target/ARM/Disassembler/ARMDisassembler.cpp:4246
+  unsigned imm =
+      fieldFromInstruction(Val, 0, 8);
+
----------------
This doesn't need to be on it's own line.


================
Comment at: llvm/lib/Target/ARM/Thumb2InstrInfo.cpp:617
       Offset += MI.getOperand(FrameRegIdx + 1).getImm();
-      NumBits = 9; // 7 bits scaled by 4
-      unsigned OffsetMask = 0x3;
+      NumBits = 7;
+      unsigned OffsetMask = 0x0;
----------------
I think it would be clearer to define the values for all three AddrModes in the switch, rather than using these defaults for one of them.


================
Comment at: llvm/test/MC/ARM/mve-load-store.s:2
+# RUN: not llvm-mc -triple=thumbv8.1m.main-none-eabi -mattr=+mve -show-encoding  < %s \
+# RUN:   | FileCheck --check-prefix=CHECK-NOFP %s
+# RUN: not llvm-mc -triple=thumbv8.1m.main-none-eabi -mattr=+mve.fp,+fp64 -show-encoding  < %s 2>%t \
----------------
None of these instructions differ with mve.fp (unless I missed some), so there's no need to duplicate the check lines. We should also be checking the error messages in the no-fp case.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D62680