[PATCH] D47775: [AArch64][SVE] Add SPLAT_VECTOR ISD Node

[James Molloy via Phabricator via llvm-commits]

jmolloy accepted this revision.
jmolloy added a comment.
This revision is now accepted and ready to land.

Thanks Graham for taking my feedback on board. I agree with Simon's request too.

I'm not convinced I agree with Roman's requests for tests on more targets; this feature is opt-in and the canonicalization is only enabled when a target marks it non-expand, which no target has apart from AArch64.



================
Comment at: llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp:17515
+  if (TLI.getOperationAction(ISD::SPLAT_VECTOR, VT) != TargetLowering::Expand)
+    if (SDValue V = (cast<BuildVectorSDNode>(N))->getSplatValue())
+      if (!V.isUndef())
----------------
nit: Extraneous parens around the cast<>.


================
Comment at: llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp:17516
+    if (SDValue V = (cast<BuildVectorSDNode>(N))->getSplatValue())
+      if (!V.isUndef())
+        return DAG.getNode(ISD::SPLAT_VECTOR, SDLoc(N), VT, V);
----------------
This is probably worth a comment. I suppose we don't convert if the splatted value is undef because the entire build_vector is undef and you haven't written all the canonicalizations for that?


================
Comment at: llvm/lib/CodeGen/SelectionDAG/LegalizeIntegerTypes.cpp:1369
+  return SDValue(DAG.UpdateNodeOperands(N,
+                                GetPromotedInteger(N->getOperand(0))), 0);
+}
----------------
I know you've copied the formatting from above, but this formatting is weird and should be clang-formatted.


================
Comment at: llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp:3558
+
+  // For now, we only handle splats for scalable vectors.
+  assert(!VT.isScalableVector() && "Unsupported scalable vector shuffle");
----------------
Can we have a comment that the DAGCombiner will perform build_vector->splat_vector conversion for non-scalable types?


================
Comment at: llvm/lib/Target/AArch64/AArch64ISelLowering.cpp:806
+    for (MVT VT : MVT::integer_scalable_vector_valuetypes()) {
+      if (isTypeLegal(VT)) {
+        if (VT.getVectorElementType() != MVT::i1) {
----------------
This if-nest is getting a bit deep, could these two ifs be merged?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D47775