[PATCH] D62672: [ARM] Add MVE vector shift instructions.

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

ostannard added inline comments.


================
Comment at: llvm/lib/Target/ARM/ARMInstrMVE.td:785
+
+def VSHLC : MVE_p<(outs rGPR:$RdmDest, MQPR:$Qd), (ins MQPR:$QdSrc, rGPR:$RdmSrc,
+                  long_shift:$imm), NoItinerary, "vshlc", "", "$QdSrc, $RdmSrc, $imm",
----------------
Why are the R and Q registers in the opposite order in the input and output operands?


================
Comment at: llvm/lib/Target/ARM/ARMInstrMVE.td:815
+
+class t2VMOVL<string iname, string suffix, bits<2> sz, bit U,
+              list<dag> pattern=[]>
----------------
Some of these names begin with "t2" (matching the other Thumb2 instructions), and some don't. Unless there's a pattern which I've missed, could pick one naming convention and stick to it?


================
Comment at: llvm/lib/Target/ARM/ARMInstrMVE.td:909
+// with this instruction then the t2 encoding will automatically get used.
+defm t1VSHLLs8 : VSHLLt1_shift_half<"vshll", "s8", 0b0, 0b0, (ins mve_shift_imm1_7:$szimm5)>;
+defm t1VSHLLu8 : VSHLLt1_shift_half<"vshll", "u8", 0b0, 0b1, (ins mve_shift_imm1_7:$szimm5)>;
----------------
These ones seem to be be using the t1/t2 prefix to distinguish between different 32-bit encodings, rather than between the 16-bit and 32-bit encodings from the reference manual. I'd prefer if we avoided using the encoding numbers at all, because these don't tend to be stable between versions of the architecture.


================
Comment at: llvm/lib/Target/ARM/ARMInstrMVE.td:1044
+
+class MVE_shift_vec<string iname, string suffix, bit U, bits<2> size,
+                    dag oops, dag iops, string ops, string cstr,
----------------
This class is only used once, can it be merged into MVE_shift_helper?


================
Comment at: llvm/lib/Target/ARM/AsmParser/ARMAsmParser.cpp:5984
 
-  if (isMnemonicVPTPredicable(Mnemonic, ExtraToken)) {
+  if (isMnemonicVPTPredicable(Mnemonic, ExtraToken) && Mnemonic != "vmovlt" &&
+      Mnemonic != "vshllt" && Mnemonic != "vrshrnt" && Mnemonic != "vshrnt" &&
----------------
What are these special cases for? Could this be moved into isMnemonicVPTPredicable?


================
Comment at: llvm/lib/Target/ARM/AsmParser/ARMAsmParser.cpp:6610
+        PredicationCode == ARMCC::LT &&
+        Mnemonic.startswith("vmov")) {
+      // Very nasty hack to deal with the vector predicated variant of vmovlt
----------------
Does this need to be an exact equality check, not just startswith? Otherwise, I think this will transform "vmov<whatever>lt" into just "vmovlt".


================
Comment at: llvm/lib/Target/ARM/Disassembler/ARMDisassembler.cpp:6089
+
+  // Depending on the (U)nsignedness, (T)op or bottom and size of the element
+  // (size16) we pick different Opcodes.  We do this using an array of opcodes
----------------
Is this not already handled by the table-generated disassembler?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D62672