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

[David Sherwood via Phabricator via llvm-commits]

david-arm added inline comments.


================
Comment at: llvm/lib/CodeGen/SelectionDAG/LegalizeVectorTypes.cpp:4022
+  SDValue IdxFactor =
+      DAG.getConstant((WidenedMinElems / MinElems), DL, MVT::i64);
+  SDValue Idx = (N->getOperand(2));
----------------
For the type here (MVT::i64) I wonder if we should be using the type returned by this:

  TLI->getVectorIdxTy(getDataLayout())

I discovered this by looking at what this function does:

  SDValue SelectionDAG::getVectorIdxConstant(uint64_t Val, const SDLoc &DL,
                                             bool isTarget) {
    return getConstant(Val, DL, TLI->getVectorIdxTy(getDataLayout()), isTarget);
  }



================
Comment at: llvm/lib/CodeGen/SelectionDAG/LegalizeVectorTypes.cpp:4023
+      DAG.getConstant((WidenedMinElems / MinElems), DL, MVT::i64);
+  SDValue Idx = (N->getOperand(2));
+
----------------
nit: I think you can drop the extra braces here.


================
Comment at: llvm/lib/CodeGen/SelectionDAG/LegalizeVectorTypes.cpp:4025
+
+  SDValue OffsetIndex = DAG.getNode(ISD::MUL, DL, MVT::i64, Idx, IdxFactor);
+  return DAG.getNode(ISD::INSERT_VECTOR_ELT, DL, WideVector.getValueType(),
----------------
Same comment as above about the type


================
Comment at: llvm/test/CodeGen/AArch64/sve-insert-element.ll:414
+}
+
----------------
Do we need to add a test for inserting into a <vscale x 1 x i1> type here?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D105119