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

[Kerry McLaughlin via Phabricator via llvm-commits]

kmclaughlin 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");
----------------
paulwalker-arm wrote:
> 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.
The only reason for this restriction was that LowerINSERT_SUBVECTOR checks that the smaller subvector is half the size of the insert_subvector before creating an unpk + uzp1. I've changed this to remove the restriction and added a loop over the number of operands instead.


================
Comment at: llvm/lib/Target/AArch64/AArch64ISelLowering.cpp:9061
+
+    if (!isTypeLegal(VT) || !Op0VT.isInteger() || !Op1VT.isInteger())
+      return SDValue();
----------------
paulwalker-arm wrote:
> 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.
I did try your suggestion here of bitcasting the operation to floating point, though I then hit another issue where the bitcast of the sub-vector (e.g. bitcasting a 2i32 truncate -> 2f32) has an operand which is not type legal.
The PromoteIntOp_BITCAST function tries to handle this by creating a store + load, but I'm not sure this is the right approach in this situation?


================
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 {
----------------
efriedma wrote:
> paulwalker-arm wrote:
> > 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?
> We could split this into a combine, I guess?
> 
> You can run DAGCombines on target-specific nodes by just adding a case to AArch64TargetLowering::PerformDAGCombine, I think.
> 
> I think we should avoid adding new target-independent nodes that are only useful for scalable vectors if we don't need them in target-independent code; it's hard to tell what common nodes would actually be useful without another backend to use them.
Added a case for UUNPKHI/LO and split this into a DAG combine


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

https://reviews.llvm.org/D86548