[ARM] Expand ROTL and ROTR of vector value types

[• chatur01]

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.

[t.p.northover]

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.

[• chatur01]

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]

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

[t.p.northover]

Thanks for updating it. I think this looks reasonable.

Tim.

[RKSimon]

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]

In D14082#275694, @RKSimon wrote:

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.

lib/Target/ARM/ARMISelLowering.cpp

if (!Subtarget->isThumb1Only()) {
     // FIXME: We should do this for Thumb1 as well.
     setOperationAction(ISD::ADDC,    MVT::i32, Custom);
     setOperationAction(ISD::ADDE,    MVT::i32, Custom);
     setOperationAction(ISD::SUBC,    MVT::i32, Custom);
     setOperationAction(ISD::SUBE,    MVT::i32, Custom);
   }
 
   // ARM does not have ROTL.
-  setOperationAction(ISD::ROTL,  MVT::i32, Expand);
+  setOperationAction(ISD::ROTL, MVT::i32, Expand);
+  for (MVT VT : MVT::vector_valuetypes()) {
+    setOperationAction(ISD::ROTL, VT, Expand);
+    setOperationAction(ISD::ROTR, VT, Expand);
+  }
   setOperationAction(ISD::CTTZ,  MVT::i32, Custom);
   setOperationAction(ISD::CTPOP, MVT::i32, Expand);
   if (!Subtarget->hasV5TOps() || Subtarget->isThumb1Only())
     setOperationAction(ISD::CTLZ, MVT::i32, Expand);
 
   // These just redirect to CTTZ and CTLZ on ARM.
   setOperationAction(ISD::CTTZ_ZERO_UNDEF  , MVT::i32  , Expand);
   setOperationAction(ISD::CTLZ_ZERO_UNDEF  , MVT::i32  , Expand);

test/CodeGen/ARM/rotate.ll

+; RUN: llc < %s -mtriple=thumbv8--linux-gnueabihf | FileCheck %s
+
+;; This used to cause a backend crash about not being able to
+;; select ROTL. Make sure if generates the basic VSHL/VSHR.
+define <2 x i64> @testcase(<2 x i64>* %in) {
+; CHECK-LABEL: testcase
+; CHECK: vshl.i64
+; CHECK: vshr.u64
+  %1 = load <2 x i64>, <2 x i64>* %in
+  %2 = lshr <2 x i64> %1, <i64 8, i64 8>
+  %3 = shl <2 x i64> %1, <i64 56, i64 56>
+  %4 = or <2 x i64> %2, %3
+  ret <2 x i64> %4
+}