Page MenuHomePhabricator

[ARM] Expand ROTL and ROTR of vector value types
ClosedPublic

Authored by chatur01 on Oct 26 2015, 10:03 AM.

Details

Summary

After D13851 landed, we saw backend crashes when compiling the reduced test case included in this patch. The right fix seems to be to allow these vector types for expansion in instruction selection.

Diff Detail

Event Timeline

chatur01 retitled this revision from to [ARM] Expand ROTL and ROTR of vector value types.
chatur01 updated this object.
chatur01 added a reviewer: rengolin.
chatur01 set the repository for this revision to rL LLVM.
chatur01 added a subscriber: llvm-commits.

Hi Charlie,

It looks like AArch64 is affected as well (but no other targets that I could find, otherwise I'd suggest making Expand the default). Are you working on a separate patch for that?

Cheers.

Tim.

Hi Tim,

Looks like I've got my blinkers on, I didn't see the crash on AArch64 in the larger test case I was analyzing. I'll put another patch up in a similar vein to the one here for ARM, just for AArch64.

Charlie.

chatur01 removed rL LLVM as the repository for this revision.

Add the same expansions on AArch64 as well. I missed that it was also bumping into this.

t.p.northover accepted this revision.Oct 26 2015, 11:30 AM
t.p.northover added a reviewer: t.p.northover.

Thanks for updating it. I think this looks reasonable.

Tim.

This revision is now accepted and ready to land.Oct 26 2015, 11:30 AM

Charlie - thanks for dealing with this - I'm surprised it didn't turn up in the fuzz testing that I've been doing recently (the XOP rotation lowering definitely caught several times). I agree with Tim that making Expand the default is probably the way forward.

chatur01 closed this revision.Oct 27 2015, 3:27 AM

Charlie - thanks for dealing with this - I'm surprised it didn't turn up in the fuzz testing that I've been doing recently (the XOP rotation lowering definitely caught several times). I agree with Tim that making Expand the default is probably the way forward.

No worries Simon. Thanks for your improvements. I've landed this change in r251401.