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

[James Molloy via Phabricator via llvm-commits]

jmolloy added inline comments.


================
Comment at: llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp:3544
 
+  if (MaskV->isNullValue() && VT.isScalableVector()) {
+    // Canonical splat form of first element of first input vector.
----------------
huntergr wrote:
> jmolloy wrote:
> > It feels to me like this is a canonicalization that should be a DAGCombine rather than in the builder. Is this here specifically because the ISD::vector_shuffle is not representable with scalable vectors?
> > 
> > If not, I'd prefer this be moved to a canonical dag combine. In general, I feel logic here should be at a minimum because it runs before targets can override or pattern match themselves.
> It's because `BUILD_VECTOR` cannot be used for scalable vectors, and if we tried to use `VECTOR_SHUFFLE`, we'd still need to generate a mask with an unknown number of elements which we can't do at the moment.
> 
> I've limited it to scalable vectors only for now precisely because this does break some unit tests if I enable it for all targets -- they do get a splat, but not the expected splat. It may be better to move this code to `SelectionDAG::getVectorShuffle` near the code that checks `TLI->hasVectorBlend()`, which also seems to match and transform splats early on.
> 
> We do have a `VECTOR_SHUFFLE_VAR` node downstream which takes a third argument as a mask, and with nodes for vscale and stepvector we're able to generate the masks we need -- but that's for a separate discussion and patch.
I suppose an extension of my comment is that this introduces a super-cool new node that a bunch of backends have wanted for a while, but doesn't plumb in any canonicalization to it for targets that aren't scalable vectors.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D47775