[PATCH] D115259: [AArch64][SVE] Lower vector.insert to predicated SEL

[Paul Walker via Phabricator via llvm-commits]

paulwalker-arm added inline comments.


================
Comment at: llvm/lib/Target/AArch64/AArch64ISelLowering.cpp:10999-11010
+  if (Idx == 0 && isPackedVectorType(VT, DAG) && !Op.getOperand(0).isUndef()) {
+    unsigned int PredPattern =
+        getSVEPredPatternFromNumElements(InVT.getVectorNumElements());
+    auto PredTy = VT.changeVectorElementType(MVT::i1);
+    SDValue PTrue = getPTrue(DAG, DL, PredTy, PredPattern);
+    SDValue ScalableVec1 = convertToScalableVector(DAG, VT, Vec1);
+    return DAG.getNode(ISD::VSELECT, DL, VT, PTrue, Vec0, ScalableVec1);
----------------
MattDevereau wrote:
> paulwalker-arm wrote:
> > Perhaps worth combining these blocks? Like
> > 
> > ```
> > if (Idx == 0 && isPackedVectorType(VT, DAG)) {
> >   // This will be matched by custom code during ISelDAGToDAG.
> >   if (Vec0.isUndef())
> >     return Op;
> > 
> >   unsigned int PredPattern = ......
> >   ......
> > }
> > ```
> The two blocks differ slightly:
> `isPackedVectorType(VT, DAG)`
> and
> `isPackedVectorType(InVT, DAG)`
> 
> I had to pass VT instead of InVT to get isPackedVector to return false on the <4 x bfloat> test. All tests pass when I combine the statements and use VT. Do you think there will be any side effects later down the line from doing this?
Oh that's interesting.  I think the original code is wrong to use `InVT` and is likely the result of some refactoring for when this code path needed to handle scalable vectors.

`isPackedVectorType` will always return true for fixed length vectors and so given by this point we know `InVT` is fixed length the call is redundant.  I believe the original code should be using `VT` because that is the only case that is handled within ISelDAGToDAG (i.e. inserting a fixed length vector into an undef packed scalable vector at index 0).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D115259