[PATCH] D100882: [AArch64] Improve vector reverse lowering

[Sander de Smalen via Phabricator via llvm-commits]

sdesmalen added inline comments.


================
Comment at: llvm/lib/Target/AArch64/AArch64ISelLowering.cpp:9038
 
+  if (V1.getValueType() == VT &&
+      ((VT.getVectorNumElements() == 8 && VT.getScalarSizeInBits() == 16) ||
----------------
dmgreen wrote:
> sdesmalen wrote:
> > nit: Is this condition necessary?
> > 
> > I know for LLVM IR nodes the result type doesn't necessarily have the same number of elements as the source vectors (but instead equals the number of elements in the mask), but is the same true for VECTOR_SHUFFLE?
> > 
> >  The reason for asking is that I see in ISDOpcodes that it says:
> >   /// VECTOR_SHUFFLE(VEC1, VEC2) - Returns a vector, of the same type as
> >   /// VEC1/VEC2.
> Yeah, I added it as an additional safety check. It didn't alter any of the test cases I had, but I figured it was better safe than sorry.
> 
> I can remove it though, if it is guaranteed that they will already match in size.
I think the sizes are supposed to be the same, looking at `SelectionDAGBuilder::visitShuffleVector` there is code that ensures the sizes match. To be sure, maybe you can add an assert instead?


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

https://reviews.llvm.org/D100882