[PATCH] D106265: [AArch64][SVE] Zero-overhead transfer between Neon and SVE registers

Tue Jul 20 01:49:11 PDT 2021

peterwaller-arm marked an inline comment as done.
peterwaller-arm added a comment.

In D106265#2887638 <https://reviews.llvm.org/D106265#2887638>, @efriedma wrote:

> Can you split the instcombine changes into a separate commit?  Or are they tied together in some way I'm missing?

Happy to split them as appropriate. However, I seek feedback on the overall approach. I believe that the instcombine changes on their own constitute a regression, which is why I kept the changes in a single patch. I will hold off splitting for a moment since I want to make sure the approach is as good as we can get. Any advice would be appreciated.

One criticism I have received surrounds the introduced TLI query isInsertSubvectorLegal, which seems inelegant. The problem is that BUILD_VECTOR creation is a generic DAGCombine, so I don't know what INSERT_SUBVECTORs can be lowered by the backend. Unfortunately <vscale x 2 x half> is marked legal, so an isTypeLegal or isOperationLegalOrCustom returns 'true', but the backend is incapable of lowering it. So one alternative solution might be to improve the INSERT_SUBVECTOR lowering.

The method as it is produces good patterns [such as (insert_subvector undef (scalar_to_vector))] but it's not possible to match them in TableGen AIUI because of the mix of scalable/fixed types, and I wasn't yet able to find something to match those patterns on as a DAGCombine which didn't result in an infinite loop. So I guess the next stop is to improve the behaviour in LowerOperation.

================
Comment at: llvm/lib/Target/AArch64/AArch64ISelLowering.h:647
+      // the full vector granule.
+      return ResVT.getSizeInBits().getKnownMinSize() ==
+             AArch64::SVEBitsPerBlock;
----------------
david-arm wrote:
> Can use getVectorMinNumElements here I think.
Discussed offline -- getVectorMinNumElements doesn't make sense as it would depend on the input type whereas the test is meant to match the assert in DAGToDAG insertSubReg.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D106265