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

[Sam Parker via Phabricator via llvm-commits]

samparker added inline comments.


================
Comment at: llvm/lib/Target/ARM/ARMInstrMVE.td:3483
+                     list<dag> pattern=[]>
+  : mve_load_store<1, 0, 1, W, 1, load,
+                   Oops,
----------------
These descriptions would be far easier to read if the load and store were separated. That would also make them easier to use when we want to apply isel patterns. I can't say that I've seen this syntax used in this way before... probably for good reason.


================
Comment at: llvm/lib/Target/ARM/ARMInstrMVE.td:3558
+
+let mayLoad = 1 in {
+defm MVE_VST : mve_vld_vst_de_wb<0b0>;
----------------
Looks like these mayLoad/Store flags have been mixed up.


================
Comment at: llvm/lib/Target/ARM/ARMInstrMVE.td:3609
+                    asm, suffix, "$Qd, $addr", "" > {
+    let IM = IndexModePost;
+    let AM = am;
----------------
Again, I'm assuming that this incorrect, IndexModeNone instead?


================
Comment at: llvm/lib/Target/ARM/ARMInstrMVE.td:3694
+                    asm, suffix, "$Qd, $addr", "" > {
+    let IM = IndexModePost;
+    let AM = am;
----------------
and here.


================
Comment at: llvm/lib/Target/ARM/ARMInstrMVE.td:3731
+foreach cond = ["", "t", "e"] in {
+  foreach suffix = [".s8", ".8", ".f8"] in {
+    def : MnemonicAlias<!strconcat("vldrb", cond , suffix),
----------------
What is f8?


================
Comment at: llvm/lib/Target/ARM/ARMInstrMVE.td:3845
+ : mve_vldr_vstr_sc1<0b0, 0b0, 0b00, 0b00, 0b0,
+                     (ins mve_addr_rq_shift<0>:$addr), "vstrb", "8">;
+def mveVSTRB16noShift
----------------
Should this 'ins' be passed as an argument? Looks like all the instructions pass it.


================
Comment at: llvm/lib/Target/ARM/ARMInstrMVE.td:3955
+defm MVE_VLDRWU32
+  : mve_vldr_vstr_sc2_addr<0b1, 0b0, (ins mve_addr_q_shift<2>:$addr), "vldrw",
+                           "u32", 2>;
----------------
Since we're already passing 'shift', it would be cleaner to construct the addrOps within the multiclass definitions. 


================
Comment at: llvm/lib/Target/ARM/ARMInstrThumb2.td:313
+// taddrmode_imm7  := reg[r0-r7] +/- (imm7 << shift)
+class TMemImm7ShiftOffsetAsmOperand<int shift>
+  : AsmOperandClass {let Name = "TMemImm7Shift"#shift#"Offset";}
----------------
Could this stuff be moved into the MVE specific td file?


================
Comment at: llvm/lib/Target/ARM/AsmParser/ARMAsmParser.cpp:1776
+    if (!Memory.OffsetImm) return true;
+    int64_t Val = Memory.OffsetImm->getValue();
+    return (Val == INT32_MIN) ||
----------------
Please extract out the shared logic into a helper.


================
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()));
----------------
either use cast if you're sure, otherwise we need to ensure CE isn't nullptr.


================
Comment at: llvm/lib/Target/ARM/AsmParser/ARMAsmParser.cpp:3063
 
+  void addTMemImm7Shift0OffsetOperands(MCInst &Inst, unsigned N) const {
+    assert(N == 2 && "Invalid number of operands!");
----------------
Do we really need all these functions that do exactly the same thing?!


================
Comment at: llvm/lib/Target/ARM/AsmParser/ARMAsmParser.cpp:4846
     // composite register classes.
-    if (Count == 2) {
+    if (Count == 2 && !hasMVE()) {
       const MCRegisterClass *RC = (Spacing == 1) ?
----------------
NoLanes and AllLanes can be folded.


================
Comment at: llvm/lib/Target/ARM/AsmParser/ARMAsmParser.cpp:7628
+    if (Qd == Qm) {
+      return Error(Operands[3]->getStartLoc(),
+                   "destination vector register and vector offset register "
----------------
Please use a helper to avoid the copy-paste.


================
Comment at: llvm/lib/Target/ARM/Disassembler/ARMDisassembler.cpp:6357
+
+  if (!Check(S, DecoderGPRRegisterClass(Inst, Rn, Address, Decoder)))
+    return MCDisassembler::Fail;
----------------
This functions share a lot of common functionality, please refactor.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D62680