[PATCH] D102498: [AArch64][SVE] Improve codegen for fixed length vector concat

[Paul Walker via Phabricator via llvm-commits]

paulwalker-arm added inline comments.


================
Comment at: llvm/lib/Target/AArch64/AArch64ISelLowering.cpp:17709
+  EVT ContainerDstVT = getContainerForFixedLengthVector(DAG, VT);
+  EVT ContainerSrcVT = getContainerForFixedLengthVector(DAG, SrcVT);
+
----------------
I'm wondering if there needs to be this distinction or if we can just have `ContainerVT`.  I say this because the container is based on the element type and since the `Src` and `Dst` element types for `CONCAT_VECTORS` must be the same, then the container type will also be the same.


================
Comment at: llvm/lib/Target/AArch64/AArch64ISelLowering.cpp:17720-17722
+  // Skip the splice for the trivial case of concat with undef
+  if (SrcOp2.isUndef())
+    Op = SrcOp1;
----------------
This looks like something that can be extracted into a target specific dag combine (i.e. `SPLICE pg, op1, undef -> op1`) being a common scalable vector transform, and means the lowering code will be simpler.  You'll see we do something similar for `AArch64ISD::UZP1`.


================
Comment at: llvm/lib/Target/AArch64/AArch64ISelLowering.cpp:17708
+  // For now don't support concat of more than 2 vectors
+  if (Op->getNumOperands() != 2)
+    return SDValue();
----------------
bsmith wrote:
> joechrisellis wrote:
> > Just a question -- can we have `CONCAT_VECTOR` with a single operand? Doesn't seem like it. 🤔 
> I don't believe so, no.
It is allowed for `CONCAT_VECTORS` to have a single operand, but this get's folded away during construction and so doesn't need special handling.  That said, I do think it's better for the `getNumOperands()` check to be done earlier before extracting specific operands via `Op.getOperand(#num)`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D102498