[PATCH] D86548: [SVE][CodeGen] Legalisation of truncate for scalable vectors

[Paul Walker via Phabricator via llvm-commits]

paulwalker-arm added a subscriber: eli.friedman.
paulwalker-arm added inline comments.


================
Comment at: llvm/lib/CodeGen/SelectionDAG/LegalizeIntegerTypes.cpp:4689-4691
+    if (N->getNumOperands() > 2)
+      report_fatal_error("Concat of more than two "
+                         "scalable vectors is not supported");
----------------
Out of interest why do we need this restriction?  The common algorithm seems like a trivial loop of the form
```
Res = getUndef(ResVT)
for i=0-NumOp; {
  Op = N->getOperand(i);
  unsigned Idx = Op.getValueType().getVectorMinNumElement();
  Res = getNode(INSERT_SUBVECTOR, ResVT, Res, Op, getConstant(Idx * i));
}
return Res;
```

I guess it's not for this patch but I'm also wondering if this could be the canonical form when type legalising CONCAT_VECTORS since the more instances of BUILD_VECTOR we can remove the better.


================
Comment at: llvm/lib/Target/AArch64/AArch64ISelLowering.cpp:9059
+    EVT VT = Op.getValueType();
+    EVT Op0VT = Op.getOperand(0).getValueType();
+
----------------
You don't need this because VT and Op0VT are the same thing, which is probably why Op1VT was named InVT, not that there's anything wrong with the rename if you prefer it.


================
Comment at: llvm/lib/Target/AArch64/AArch64ISelLowering.cpp:9061
+
+    if (!isTypeLegal(VT) || !Op0VT.isInteger() || !Op1VT.isInteger())
+      return SDValue();
----------------
Probably a bit left field but the issue here is the integer sub-vector is not type legal.  Is it possible to bitcast the operation to floating point, then we just need to add patterns to  InstrInfo.td.

I can imagine DAGCombine might undo the transform but it's perhaps worth an experiment, even if it's after landing the patch since I imagine we'll want to support floating-point inserts at some point.


================
Comment at: llvm/lib/Target/AArch64/AArch64ISelLowering.cpp:9081-9082
+      HalfVec0 = DAG.getUNDEF(WideVT);
+    else if (Vec0.getOpcode() == AArch64ISD::UZP1)
+      HalfVec0 = Vec0->getOperand(0);
+    else {
----------------
FYI: This looks more like a DAGCombine.  Considering how much we're relying on UPK/UZP for legalisation I'm wondering if we should (separately from this patch) introduce common ISD nodes for these operations.  Assuming there isn't already a way to do combines for target nodes? What does @eli.friedman think?


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

https://reviews.llvm.org/D86548