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

[Hao Liu]

Hi Tim,

I've refactored the patch and added it again:
http://llvm-reviews.chandlerc.com/D1885.

Thanks,
-Hao

-----Original Message-----
From: cfe-commits-bounces at cs.uiuc.edu
[mailto:cfe-commits-bounces at cs.uiuc.edu] On Behalf Of Tim Northover
Sent: Wednesday, October 09, 2013 5:50 PM
To: t.p.northover at gmail.com; haoliuts at gmail.com
Cc: llvm-commits at cs.uiuc.edu; cfe-commits at cs.uiuc.edu
Subject: Re: [PATCH] [AArch64] implement aarch64 neon vector load/store
N-element structure class AdvSIMD (lselem)


  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
_______________________________________________
cfe-commits mailing list
cfe-commits at cs.uiuc.edu
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
-------------- next part --------------
A non-text attachment was scrubbed...
Name: llvm-ldst-multi-v3.1.patch
Type: application/octet-stream
Size: 152744 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20131010/684859ce/attachment.obj>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: clang-ldst-multi-v3.patch
Type: application/octet-stream
Size: 41856 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20131010/684859ce/attachment-0001.obj>