[PATCH] D85364: [SVE][WIP] Implement lowering for fixed width select

[Eli Friedman via Phabricator via llvm-commits]

efriedma added inline comments.


================
Comment at: llvm/lib/Target/AArch64/AArch64ISelLowering.cpp:7248
     if (SrcVT.getSizeInBits() < VT.getSizeInBits()) {
       assert(2 * SrcVT.getSizeInBits() == VT.getSizeInBits());
       // We can pad out the smaller vector for free, so if it's part of a
----------------
Is this assert also a potential issue?


================
Comment at: llvm/lib/Target/AArch64/AArch64ISelLowering.cpp:7286
 
       Src.ShuffleVec = DAG.getNode(AArch64ISD::EXT, dl, DestVT, VEXTSrc1,
                                    VEXTSrc2,
----------------
Is AArch64ISD::EXT going to work with arbitrarily large vectors?


================
Comment at: llvm/lib/Target/AArch64/AArch64ISelLowering.cpp:7257
 
-    assert(SrcVT.getSizeInBits() == 2 * VT.getSizeInBits());
+    assert(SrcVT.getSizeInBits() <= 4 * VT.getSizeInBits());
 
----------------
cameron.mcinally wrote:
> @t.p.northover, does this change look okay to you? I suspect that this assert is assuming we only have NEON 64b and 128b vectors.
> 
> Fixed width SVE now has larger vectors, and for these particular tests we want to extract a 1/4 width subvector from a single width vector.
Why is 4 special here?


================
Comment at: llvm/lib/Target/AArch64/AArch64ISelLowering.cpp:9045
+      InVT.getSizeInBits() == 128)
     return Op;
 
----------------
cameron.mcinally wrote:
> Just checking if anyone sees a problem with this change. We need to explicitly check that this is a 128b vector now, since SVE can see larger fixed width vectors.
This looks fine.


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

https://reviews.llvm.org/D85364