[PATCH] D58927: [ARM] Fixed an assumption of power-of-2 vector MVT

[Diogo N. Sampaio via Phabricator via llvm-commits]

dnsampaio added inline comments.


================
Comment at: lib/Target/ARM/ARMISelLowering.cpp:12153
   unsigned NumLanes = Op.getValueType().getVectorNumElements();
-  if (FloatBits != 32 || IntBits > 32 || NumLanes > 4) {
+  if (FloatBits != 32 || IntBits > 32 || NumLanes > 4 || NumLanes == 3) {
     // These instructions only exist converting from f32 to i32. We can handle
----------------
tpr wrote:
> RKSimon wrote:
> > Is this more readable? I'm not sure if you need to permit NumLanes == 1 or not.
> > 
> > FloatBits != 32 || IntBits > 32 || (NumLanes != 4 && NumLanes != 2)
> I am also not sure whether it needs to permit NumLanes==1. So I think my fix is less intrusive, as it does not matter that I do not know. Is that ok?
The code explicitly assume that it had either 2 or 4 lanes as you can see here: `NumLanes == 2 ? MVT::v2i32 : MVT::v4i32`, and many other places where the NumLanes is divided by 2 just below here.
I believe Simon's approach is the correct one.


Repository:
  rL LLVM

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

https://reviews.llvm.org/D58927