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

[tpr]

Summary

I am about to introduce some non-power-of-2 width vector MVTs. This
commit fixes a power-of-2 assumption that my forthcoming change would
otherwise break.

Change-Id: I56a282e365d3874ab0621e5bdef98a612f702317

[kristof.beyls]

Hi Tim,

It seems to me that this could use a test-case that fails without this fix.

[RKSimon]

lib/Target/ARM/ARMISelLowering.cpp

12153

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)

[tpr]

In D58927#1422518, @kristof.beyls wrote:

Hi Tim,

It seems to me that this could use a test-case that fails without this fix.

Hi Kristof

I can't make a test fail here, because v3i32/v3f32 are not yet MVTs. With my change to add them as MVTs, and without this commit here, I get test failures in test/CodeGen/ARM/vcvt_combine.ll and vdiv_combine.ll.

I guess I should have mentioned that in the original commit message. :-) I will include it when I land it.

lib/Target/ARM/ARMISelLowering.cpp

12153

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?

[dnsampaio]

lib/Target/ARM/ARMISelLowering.cpp

12153

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.

[tpr]

V3: Addressed review comment.

[tpr]

lib/Target/ARM/ARMISelLowering.cpp

12153

Good point; fixed now.

[dnsampaio]

lib/Target/ARM/ARMISelLowering.cpp

12211

Replicate it here?

[RKSimon]

lib/Target/ARM/ARMISelLowering.cpp

12211

if (FloatBits != 32 || IntBits > 32 || (NumLanes != 4 && NumLanes != 2)

Update comment below as well please.

[tpr]

V4: Properly addressed review comment.

[tpr]

lib/Target/ARM/ARMISelLowering.cpp

12211

Oops, sorry, forgot the second one.

[RKSimon]

LGTM

lib/Target/ARM/ARMISelLowering.cpp

static SDValue PerformVCVTCombine(SDNode *N, SelectionDAG &DAG,
   if (!isa<BuildVectorSDNode>(ConstVec))
     return SDValue();
 
   MVT FloatTy = Op.getSimpleValueType().getVectorElementType();
   uint32_t FloatBits = FloatTy.getSizeInBits();
   MVT IntTy = N->getSimpleValueType(0).getVectorElementType();
   uint32_t IntBits = IntTy.getSizeInBits();
   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
     // smaller integers by generating an extra truncate, but larger ones would
     // be lossy. We also can't handle more then 4 lanes, since these intructions
     // only support v2i32/v4i32 types.
     return SDValue();
   }
 
   BitVector UndefElements;
static SDValue PerformVDIVCombine(SDNode *N, SelectionDAG &DAG,
   if (!isa<BuildVectorSDNode>(ConstVec))
     return SDValue();
 
   MVT FloatTy = N->getSimpleValueType(0).getVectorElementType();
   uint32_t FloatBits = FloatTy.getSizeInBits();
   MVT IntTy = Op.getOperand(0).getSimpleValueType().getVectorElementType();
   uint32_t IntBits = IntTy.getSizeInBits();
   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 i32 to f32. We can handle
     // smaller integers by generating an extra extend, but larger ones would
     // be lossy. We also can't handle more then 4 lanes, since these intructions
     // only support v2i32/v4i32 types.
     return SDValue();
   }
 
   BitVector UndefElements;