[PATCH] D105119: [SVE] Fix incorrect codegen when inserting vector elements into widened scalable vectors

[Sander de Smalen via Phabricator via llvm-commits]

sdesmalen added a comment.

In D105119#2848107 <https://reviews.llvm.org/D105119#2848107>, @efriedma wrote:

> For non-scalable vectors, widening means we pad the end with undef elements (i.e. INSERT_SUBVECTOR into an undef vector).  Do you want it to mean something different for scalable vectors?  Is this documented somewhere?

The difference is that scalable vectors are unpacked, so for `<vscale x 1 x i64>` <=> `<v0 | v1 | .... | vn-1>`, needs widening to `<vscale x 2 x i64>` <=> `<v0, _ | v1, _, | ... | vn-1, _>`, which means that the index in which to insert the element needs to be multiplied by 2. For fixed-width vectors, we'd indeed pad the number of elements in the vector to go from `<2 x i32> <v0, v1>` to `<4 x i32> <v0, v1, _, _>`, so the index stays the same. I'm not really sure where this should be documented as it's mostly a characteristic of scalable vectors, but I guess an extra comment describing it here wouldn't hurt. Note that the widening only works for `vscale x 1` because it is a power of 2. I'm not sure if it would even be possible to widen `<vscale x 3` to `<vscale x 4`.



================
Comment at: llvm/lib/CodeGen/SelectionDAG/LegalizeVectorTypes.cpp:4009
+  if (InVector.getValueType().isScalableVector()) {
+    unsigned MinElems = InVector.getValueType().getVectorMinNumElements();
+    unsigned WidenedMinElems =
----------------
Can you add an assert that MinElems is a power of 2?
And also that the widened vector is scalable as well?


================
Comment at: llvm/lib/CodeGen/SelectionDAG/LegalizeVectorTypes.cpp:4018
+    SDValue OffsetIndex = DAG.getNode(ISD::MUL, DL, MVT::i64, Idx, IdxFactor);
+
+    return DAG.getNode(ISD::INSERT_VECTOR_ELT, DL, WideVector.getValueType(),
----------------
nit: redundant whitespace


================
Comment at: llvm/lib/CodeGen/SelectionDAG/LegalizeVectorTypes.cpp:4022
+  }
+  return DAG.getNode(ISD::INSERT_VECTOR_ELT, DL, WideVector.getValueType(),
+                     WideVector, N->getOperand(1), N->getOperand(2));
----------------
nit: maybe handle this case first, so you don't need the indentation for the scalable case.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D105119