[PATCH] [AArch64] implement aarch64 neon vector load/store N-element structure class AdvSIMD (lselem)

[Tim Northover]

  Hi Hao,

  These are starting to look really good. Quite a few points, but they're all very minor now, I think the overall approach is about as neat as you can get it.

  One other thing (not tied to any particular diff line), there don't appear to be any LLVM IR CodeGen tests; you should add some of those.

  Cheers.

  Tim.


================
Comment at: lib/Target/AArch64/AArch64ISelLowering.h:284-286
@@ -283,2 +283,5 @@
   getRegForInlineAsmConstraint(const std::string &Constraint, MVT VT) const;
+
+  virtual bool getTgtMemIntrinsic(IntrinsicInfo &Info, const CallInst &I,
+                                  unsigned Intrinsic) const;
 private:
----------------
I think there's an LLVM_OVERRIDE macro that would let us mark this in case the prototype changes (in C++11 mode).

================
Comment at: lib/Target/AArch64/AArch64InstrNEON.td:3029-3032
@@ +3028,6 @@
+// Vector List Operand with 1/2/3/4 registers
+defm VOne : VectorList_BHSD<"VOne", 1, FPR64, FPR128>;
+defm VPair : VectorList_BHSD<"VPair", 2, DPair, QPair>;
+defm VTriple : VectorList_BHSD<"VTriple", 3, DTriple, QTriple>;
+defm VQuad : VectorList_BHSD<"VQuad", 4, DQuad, QQuad>;
+
----------------
I think these should be defined in AArch64RegisterInfo.td

================
Comment at: lib/Target/AArch64/AArch64InstrNEON.td:3041
@@ +3040,3 @@
+                 NoItinerary> {
+  let mayLoad = 1;
+}
----------------
You'll want to check this, but I think TableGen will be inferring "hasSideEffects" on these instructions without a pattern. If so you should add a "let hasSideEffects = 0" line.

================
Comment at: lib/Target/AArch64/AArch64RegisterInfo.td:210-211
@@ +209,4 @@
+// 2 Consecutive 64-bit registers: D0_D1, D1_D2, ..., D30_D31
+def Tuples2D : RegisterTuples<[dsub_0, dsub_1],
+                              [(rotl FPR64, 0), (rotl FPR64, 1)]>;
+                              
----------------
I like the improvement here, using rotl instead of extra defs. Well done.

================
Comment at: lib/Target/AArch64/AsmParser/AArch64AsmParser.cpp:1986
@@ +1985,3 @@
+    if (Space == 0 || Space > 3 || LayoutStr != LayoutStr2) {
+      Error(RegLoc2, "invalid operand for instruction");
+      return MatchOperand_ParseFail;
----------------
This message could be improved. Custom Parsers are ugly, but the one benefit they do have is very good error reporting, so we should make use of it.

================
Comment at: lib/Target/AArch64/AsmParser/AArch64AsmParser.cpp:2009
@@ +2008,3 @@
+      if (Space != 1 || LayoutStr != LayoutStr2 || Count > 4) {
+        Error(RegLoc2, "invalid operand for instruction");
+        return MatchOperand_ParseFail;
----------------
Same comment about error message

================
Comment at: lib/Target/AArch64/AsmParser/AArch64AsmParser.cpp:2029-2041
@@ +2028,15 @@
+    bool IsVec64 = (Layout < A64Layout::_16B) ? true : false;
+    switch (Count) {
+    case 2:
+      SupRegID =
+          (IsVec64) ? AArch64::DPairRegClassID : AArch64::QPairRegClassID;
+      break;
+    case 3:
+      SupRegID =
+          (IsVec64) ? AArch64::DTripleRegClassID : AArch64::QTripleRegClassID;
+      break;
+    case 4:
+      SupRegID =
+          (IsVec64) ? AArch64::DQuadRegClassID : AArch64::QQuadRegClassID;
+      break;
+    }
----------------
A static array might improve this:

    static unsigned SupRegIDs[][] = {{AArch64::QPairClassID, AArch64::DPairClassID}, ...};
    SupRegID = SupRegIDs[Count-2][static_cast<int>(IsVec64)];

It's borderline though. Entirely up to you, just another option.

================
Comment at: lib/Target/AArch64/Disassembler/AArch64Disassembler.cpp:364-373
@@ -363,1 +363,12 @@
 
+static DecodeStatus DecodeDPairRegisterClass(llvm::MCInst &Inst, unsigned RegNo,
+                                             uint64_t Address,
+                                             const void *Decoder) {
+  if (RegNo > 31)
+    return MCDisassembler::Fail;
+
+  uint16_t Register = getReg(Decoder, AArch64::DPairRegClassID, RegNo);
+  Inst.addOperand(MCOperand::CreateReg(Register));
+  return MCDisassembler::Success;
+}
+
----------------
Could most of these be 1-line forwards, passing the RegClassID as an extra parameter?


http://llvm-reviews.chandlerc.com/D1869