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

[Simon Tatham via Phabricator via llvm-commits]

simon_tatham marked 2 inline comments as done.
simon_tatham added a comment.

(I'm still working on the rest of your review comments)



================
Comment at: llvm/lib/Target/ARM/AsmParser/ARMAsmParser.cpp:2636
+    assert(N == 1 && "Invalid number of operands!");
+    const MCConstantExpr *CE = dyn_cast<MCConstantExpr>(getImm());
+    Inst.addOperand(MCOperand::createImm(CE->getValue()));
----------------
samparker wrote:
> either use cast if you're sure, otherwise we need to ensure CE isn't nullptr.
That hardly seems fair – all the //other// functions on both sides of this hunk are written in the same style! But I'll add an assertion to just these ones if you like :-)


================
Comment at: llvm/lib/Target/ARM/AsmParser/ARMAsmParser.cpp:3063
 
+  void addTMemImm7Shift0OffsetOperands(MCInst &Inst, unsigned N) const {
+    assert(N == 2 && "Invalid number of operands!");
----------------
samparker wrote:
> Do we really need all these functions that do exactly the same thing?!
I think all the //names// have to exist, because the Tablegen-generated code will want to call them all by concatenating the innermost operand name (`Imm7Shift1` or similar) into the middle of a standard function-name template. But I can turn them all into trivial wrappers on a single function body.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D62680