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

Thu Sep 3 08:40:35 PDT 2020

cameron.mcinally 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
----------------
efriedma wrote:
> Is this assert also a potential issue?
Not sure. The fixed-width lowering case should never get here. Splitting will only be taking a subvector out of a larger vector with the same element type.

================
Comment at: llvm/lib/Target/AArch64/AArch64ISelLowering.cpp:7286

       Src.ShuffleVec = DAG.getNode(AArch64ISD::EXT, dl, DestVT, VEXTSrc1,
                                    VEXTSrc2,
----------------
efriedma wrote:
> Is AArch64ISD::EXT going to work with arbitrarily large vectors?
The fixed-width lowering case should never get here. We always have one source that's being split.

But, yeah, I agree that this is a little sketchy. Maybe we should have a separate routine to lower the splitting case? I don't know. Just thinking out loud.

Or maybe we should have a special 1 source case within ReconstructShuffle(...)? That could work.

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

https://reviews.llvm.org/D85364