[SelectionDAG] Fix incorrect offset when expanding CONCAT_VECTORS.

[paulwalker-arm]

Summary

ExpandVectorBuildThroughStack is also used for CONCAT_VECTORS.
However, when calculating the offsets for each of the operands we
incorrectly use the element size rather than actual size and thus
the stores overlap.

[paulwalker-arm]

llvm/test/CodeGen/AArch64/sve-fixed-length-shuffles.ll

14

The tests are a little looser than I'd like but this mov prevents me from using CHECK-DAG because there's a similar unrelated mov that's part of the function prolog.

[cameron.mcinally]

LGTM

llvm/lib/CodeGen/SelectionDAG/LegalizeDAG.cpp

1429

Nit: Not specific to this patch, but I think we can hoist Node->getOperand(i).getValueType() out of the loop. All the BUILD_VECTOR/CONCAT_VECTOR operand types should be the same.

Looking deeper, the BUILD_VECTOR description is a little vague though:

/// The types of the operands must all be
/// the same and must match the vector element type, except that integer types
/// are allowed to be larger than the element type, in which case the operands
/// are implicitly truncated.

I assume the larger integer operand types must all be the same type. Maybe I'm misinterpreting this though.

Just queried llvm-dev about BUILD_VECTOR and will report back...

[cameron.mcinally]

llvm/lib/CodeGen/SelectionDAG/LegalizeDAG.cpp

1429

It seems that the operands must always have the same type, but there's a modicum of disagreement (uncertainty?) there. No reason to hold up this patch though.

[RKSimon]

@paulwalker-arm Does this finally fix PR12772?

[paulwalker-arm]

In D83303#2137371, @RKSimon wrote:

@paulwalker-arm Does this finally fix PR12772?

@RKSimon: I believe so. I'll update the ticket once I've pushed this change.

[JonChesterfield]

Nice, thank you. @davidb this probably means you can drop the TargetConcatVectors ISD node from your llc.

llvm/lib/CodeGen/SelectionDAG/LegalizeDAG.cpp

Ch = DAG.getStore(
       Ch, dl, Part, SubStackPtr,
       MachinePointerInfo::getUnknownStack(DAG.getMachineFunction()));
 
   // Finally, load the updated vector.
   return DAG.getLoad(Op.getValueType(), dl, Ch, StackPtr, PtrInfo);
 }
 
 SDValue SelectionDAGLegalize::ExpandVectorBuildThroughStack(SDNode* Node) {
+  assert((Node->getOpcode() == ISD::BUILD_VECTOR ||
+          Node->getOpcode() == ISD::CONCAT_VECTORS) &&
+         "Unexpected opcode!");
+
   // We can't handle this case efficiently.  Allocate a sufficiently
-  // aligned object on the stack, store each element into it, then load
+  // aligned object on the stack, store each operand into it, then load
   // the result as a vector.
   // Create the stack frame object.
   EVT VT = Node->getValueType(0);
-  EVT EltVT = VT.getVectorElementType();
+  EVT MemVT = isa<BuildVectorSDNode>(Node) ? VT.getVectorElementType()
+                                           : Node->getOperand(0).getValueType();
   SDLoc dl(Node);
   SDValue FIPtr = DAG.CreateStackTemporary(VT);
   int FI = cast<FrameIndexSDNode>(FIPtr.getNode())->getIndex();
   MachinePointerInfo PtrInfo =
       MachinePointerInfo::getFixedStack(DAG.getMachineFunction(), FI);
 
   // Emit a store of each element to the stack slot.
   SmallVector<SDValue, 8> Stores;
-  unsigned TypeByteSize = EltVT.getSizeInBits() / 8;
+  unsigned TypeByteSize = MemVT.getSizeInBits() / 8;
   assert(TypeByteSize > 0 && "Vector element type too small for stack store!");
   // Store (in the right endianness) the elements to memory.
   for (unsigned i = 0, e = Node->getNumOperands(); i != e; ++i) {
     // Ignore undef elements.
     if (Node->getOperand(i).isUndef()) continue;
 
     unsigned Offset = TypeByteSize*i;
 
     SDValue Idx = DAG.getMemBasePlusOffset(FIPtr, Offset, dl);
 
     // If the destination vector element type is narrower than the source
     // element type, only store the bits necessary.
-    if (EltVT.bitsLT(Node->getOperand(i).getValueType().getScalarType())) {
+    if (MemVT.bitsLT(Node->getOperand(i).getValueType()))
       Stores.push_back(DAG.getTruncStore(DAG.getEntryNode(), dl,
                                          Node->getOperand(i), Idx,
-                                         PtrInfo.getWithOffset(Offset), EltVT));
-    } else
+                                         PtrInfo.getWithOffset(Offset), MemVT));
+    else
       Stores.push_back(DAG.getStore(DAG.getEntryNode(), dl, Node->getOperand(i),
                                     Idx, PtrInfo.getWithOffset(Offset)));
   }
 
   SDValue StoreChain;
   if (!Stores.empty())    // Not all undef elements?
     StoreChain = DAG.getNode(ISD::TokenFactor, dl, MVT::Other, Stores);
   else

llvm/test/CodeGen/AArch64/sve-fixed-length-shuffles.ll

+; RUN: llc -aarch64-sve-vector-bits-min=256 < %s | FileCheck %s
+; RUN: llc -aarch64-sve-vector-bits-min=512 < %s | FileCheck %s -check-prefixes=CHECK,VBITS_GE_512
+
+target triple = "aarch64-unknown-linux-gnu"
+
+; NOTE: Currently all CONCAT_VECTORS get expanded so there's little point in
+; validating all combinations of vector type.
+
+define void @concat_vectors_v4i64(<2 x i64> %a, <2 x i64> %b, <4 x i64> *%c.addr) #0 {
+; CHECK-LABEL: concat_vectors_v4i64:
+; CHECK: stp q0, q1, [sp]
+; CHECK: ptrue [[OUT_PG:p[0-9]+]].d, vl4
+; CHECK: mov x[[LO_ADDR:[0-9]+]], sp
+; CHECK: ld1d { z{{[0-9]+}}.d }, [[OUT_PG]]/z, [x[[LO_ADDR]]]
+  %concat = shufflevector <2 x i64> %a, <2 x i64> %b, <4 x i32> <i32 0, i32 1, i32 2, i32 3>
+  store <4 x i64> %concat, <4 x i64>* %c.addr
+  ret void
+}
+
+define void @concat_vectors_v8i64(<4 x i64> *%a.addr, <4 x i64> *%b.addr, <8 x i64> *%c.addr) #0 {
+; VBITS_GE_512-LABEL: concat_vectors_v8i64:
+; VBITS_GE_512: ptrue [[IN_PG:p[0-9]+]].d, vl4
+; VBITS_GE_512: ld1d { [[LO:z[0-9]+]].d }, [[IN_PG]]/z, [x0]
+; VBITS_GE_512: ld1d { [[HI:z[0-9]+]].d }, [[IN_PG]]/z, [x1]
+; VBITS_GE_512: mov x[[LO_ADDR:[0-9]+]], sp
+; VBITS_GE_512: orr x[[HI_ADDR:[0-9]+]], x[[LO_ADDR]], #0x20
+; VBITS_GE_512: st1d { [[LO]].d }, [[IN_PG]], [x[[LO_ADDR]]]
+; VBITS_GE_512: st1d { [[HI]].d }, [[IN_PG]], [x[[HI_ADDR]]]
+; VBITS_GE_512: ptrue [[OUT_PG:p[0-9]+]].d, vl8
+; VBITS_GE_512: ld1d { z{{[0-9]+}}.d }, [[OUT_PG]]/z, [x8]
+  %a = load <4 x i64>, <4 x i64>* %a.addr
+  %b = load <4 x i64>, <4 x i64>* %b.addr
+  %concat = shufflevector <4 x i64> %a, <4 x i64> %b, <8 x i32> <i32 0, i32 1, i32 2, i32 3, i32 4, i32 5, i32 6, i32 7>
+  store <8 x i64> %concat, <8 x i64>* %c.addr
+  ret void
+}
+
+attributes #0 = { nounwind "target-features"="+sve" }