Page MenuHomePhabricator

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

Authored by tpr on Mar 4 2019, 3:15 PM.

Details

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

Diff Detail

Repository
rL LLVM

Event Timeline

tpr created this revision.Mar 4 2019, 3:15 PM

Hi Tim,

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

RKSimon added inline comments.Mar 8 2019, 9:55 AM
lib/Target/ARM/ARMISelLowering.cpp
12153 ↗(On Diff #189214)

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 marked an inline comment as done.Mar 11 2019, 5:40 AM

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 ↗(On Diff #189214)

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 added inline comments.Mar 12 2019, 2:47 AM
lib/Target/ARM/ARMISelLowering.cpp
12153 ↗(On Diff #189214)

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 updated this revision to Diff 190385.Mar 13 2019, 2:55 AM

V3: Addressed review comment.

tpr marked 3 inline comments as done.Mar 13 2019, 2:56 AM
tpr added inline comments.
lib/Target/ARM/ARMISelLowering.cpp
12153 ↗(On Diff #189214)

Good point; fixed now.

dnsampaio added inline comments.Mar 13 2019, 3:01 AM
lib/Target/ARM/ARMISelLowering.cpp
12215 ↗(On Diff #190385)

Replicate it here?

RKSimon added inline comments.Mar 13 2019, 1:54 PM
lib/Target/ARM/ARMISelLowering.cpp
12215 ↗(On Diff #190385)

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

Update comment below as well please.

tpr updated this revision to Diff 190956.Mar 16 2019, 6:07 AM
tpr marked an inline comment as done.

V4: Properly addressed review comment.

tpr marked 3 inline comments as done.Mar 16 2019, 6:07 AM
tpr added inline comments.
lib/Target/ARM/ARMISelLowering.cpp
12215 ↗(On Diff #190385)

Oops, sorry, forgot the second one.

RKSimon accepted this revision.Mar 16 2019, 6:24 AM

LGTM

This revision is now accepted and ready to land.Mar 16 2019, 6:24 AM
This revision was automatically updated to reflect the committed changes.