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

[Tim Renouf via Phabricator via llvm-commits]

tpr marked an inline comment as done.
tpr added a comment.

In D58927#1422518 <https://reviews.llvm.org/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.



================
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
----------------
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?


Repository:
  rL LLVM

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

https://reviews.llvm.org/D58927