Re: [PATCH] Implement aarch64 neon instructio​n class SIMD lsone and lsone-post - LLVM

[Tim Northover]

================
Comment at: lib/Target/AArch64/AArch64ISelDAGToDAG.cpp:763
@@ +762,3 @@
+                        Operand,
+                        CurDAG->getTargetConstant(AArch64::sub_64, MVT::i32));
+  return SDValue(Reg, 0);
----------------
Should this be SRIdx? If not, that variable is unused. It's a little worrying that this passes its tests actually.

================
Comment at: lib/Target/AArch64/AArch64ISelDAGToDAG.cpp:777
@@ +776,3 @@
+  default: llvm_unreachable("unhandled vld-dup type");
+  case MVT::v8i8:  OpcodeIndex = 0; break;
+  case MVT::v4i16: OpcodeIndex = 1; break;
----------------
This OpcodeIndex mapping is horribly non-obvious and hard-coded. It also seems to omit any f16 vectors, which seems odd.

I'd suggest separate 64-bit and 128-bit tables, and basing the index off of Log2_32(VT.getScalarType().getSizeInBits()) or some other way to avoid enumerating all types.

================
Comment at: lib/Target/AArch64/AArch64ISelDAGToDAG.cpp:803
@@ +802,3 @@
+
+  std::vector<EVT> ResTys;
+  ResTys.push_back(MVT::Untyped); // Type of result super register
----------------
SmallVector would be better. Or perhaps a ?: with a call to SelectionDAG::getSDVTList.

================
Comment at: lib/Target/AArch64/AArch64ISelDAGToDAG.cpp:856
@@ +855,3 @@
+  default: llvm_unreachable("unhandled vld/vst lane type");
+  case MVT::v16i8: OpcodeIndex = 0; break;
+  case MVT::v8i16: OpcodeIndex = 1; break;
----------------
Same odd OpcodeIndex.

================
Comment at: lib/Target/AArch64/AArch64ISelDAGToDAG.cpp:885
@@ +884,3 @@
+    for (unsigned i = 0; i < Regs.size(); i++)
+      Regs[i] = getTargetSubregToReg(AArch64::sub_64, dl, VT, VT64, Regs[i]);
+  SDValue SuperReg = createQTuple(Regs);
----------------
Ah, I see why that function works now.

================
Comment at: lib/Target/AArch64/AArch64ISelDAGToDAG.cpp:833
@@ +832,3 @@
+SDNode *AArch64DAGToDAGISel::SelectVLDSTLane(SDNode *N, bool IsLoad,
+                                             unsigned NumVecs, bool isUpdating,
+                                             const uint16_t *Opcodes) {
----------------
"IsUpdating" according to coding standards. Especially as it's right next to "IsLoad" with the capital

================
Comment at: lib/Target/AArch64/AArch64ISelDAGToDAG.cpp:883
@@ +882,3 @@
+                               N->op_begin() + Vec0Idx + NumVecs);
+  if (is64BitVector)
+    for (unsigned i = 0; i < Regs.size(); i++)
----------------
I tend to think this complexity ought to be handled in the front-end (like the legacy TBL/TBX you did recently): a shufflvector extension before calling  bona fide 128-bit intrinsic.

================
Comment at: lib/Target/AArch64/AArch64ISelDAGToDAG.cpp:838
@@ +837,3 @@
+  unsigned AddrOpIdx = isUpdating ? 1 : 2;
+  unsigned Vec0Idx = 3; // AddrOpIdx + (isUpdating ? 2 : 1)
+
----------------
Commented code.


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