Fix shift legalization and lowering for big constants.

[chfast]

Summary

If shift amount is a constant value > 64 bit it is handled incorrectly during type legalization and X86 lowering. This patch the type of shift amount argument in function DAGTypeLegalizer::ExpandShiftByConstant from unsigned to APInt.

[chfast]

Ping.

[chfast]

Ping².

[sanjoy]

I don't see anything wrong with the change, but I don't understand the ins and outs (no pun intended!) of SelectionDAG well enough to give an explicit LGTM.

[chfast]

Ok, thanks, I will wait some more.

The change is not much about SelectionDAG. I just want to avoid calling getZExtValue() at line 2180 when the value of the APInt cannot fit 64 bits.

[chfast]

However, I would be very happy if I could commit that before 3.7 release is branched.

[RKSimon]

Hi Paweł - comments below.

lib/CodeGen/SelectionDAG/LegalizeIntegerTypes.cpp

2180

I'm not sure about this - ExpandShiftByConstant already handles shift amounts out of range, setting the result to zero/signbit for logical/arithmetic shifts instead of undef.

I think it better to change ExpandShiftByConstant shift amount to take an APInt type instead of an unsigned and update its handling to compare using APInt.

If you want to make a case for returning undef for constant shift results it should be in a separate patch.

test/CodeGen/X86/legalize-shl-vec.ll

3

It would be better if these tests checked the resultant codegen, rather than just test for a crash.

[chfast]

Thanks Simon for the comments. I will do as you suggested.

[chfast]

I changed DAGTypeLegalizer::ExpandShiftByConstant to accept APInt as the shift amount.
There is one quirk in that: because there is not operator- for pair (uint64_t, APInt)
I workarounded it with strage looking -Amt + NVTBits.

[RKSimon]

LGTM, thanks.

lib/CodeGen/SelectionDAG/LegalizeIntegerTypes.cpp

 
 void DAGTypeLegalizer::ExpandIntRes_Shift(SDNode *N,
                                           SDValue &Lo, SDValue &Hi) {
   EVT VT = N->getValueType(0);
   SDLoc dl(N);
 
   // If we can emit an efficient shift operation, do so now.  Check to see if
   // the RHS is a constant.
-  if (ConstantSDNode *CN = dyn_cast<ConstantSDNode>(N->getOperand(1)))
+  if (ConstantSDNode *CN = dyn_cast<ConstantSDNode>(N->getOperand(1))) {
+    if (CN->getAPIntValue().uge(VT.getSizeInBits())) {
+      SplitRes_UNDEF(N, Lo, Hi);
+      return;
+    }
     return ExpandShiftByConstant(N, CN->getZExtValue(), Lo, Hi);
+  }
 
   // If we can determine that the high bit of the shift is zero or one, even if
   // the low bits are variable, emit this shift in an optimized form.
   if (ExpandShiftWithKnownAmountBit(N, Lo, Hi))
     return;
 
   // If this target supports shift_PARTS, use it.  First, map to the _PARTS opc.
   unsigned PartsOpc;

lib/Target/X86/X86ISelLowering.cpp

static SDValue PerformSHLCombine(SDNode *N, SelectionDAG &DAG) {
   // shl.
   // (shl V, 1) -> add V,V
   if (auto *N1BV = dyn_cast<BuildVectorSDNode>(N1))
     if (auto *N1SplatC = N1BV->getConstantSplatNode()) {
       assert(N0.getValueType().isVector() && "Invalid vector shift type");
       // We shift all of the values by one. In many cases we do not have
       // hardware support for this operation. This is better expressed as an ADD
       // of two values.
-      if (N1SplatC->getZExtValue() == 1)
+      if (N1SplatC->getAPIntValue() == 1)
         return DAG.getNode(ISD::ADD, SDLoc(N), VT, N0, N0);
     }
 
   return SDValue();
 }
 
 /// \brief Returns a vector of 0s if the node in input is a vector logical
 /// shift by a constant amount which is known to be bigger than or equal

test/CodeGen/X86/legalize-shl-vec.ll

+; RUN: llc < %s -march=x86-64 | FileCheck %s
+; RUN: llc < %s -march=x86    | FileCheck %s
+
+; CHECK-LABEL: test1
+define <2 x i256> @test1(<2 x i256> %In) {
+  %Amt = insertelement <2 x i256> undef, i256 -1, i32 0
+  %Out = shl <2 x i256> %In, %Amt
+  ret <2 x i256> %Out
+}
+
+; CHECK-LABEL: test2
+define <2 x i256> @test2(<2 x i256> %In) {
+  %Amt = insertelement <2 x i256> undef, i256 -1, i32 0
+  %Out = ashr <2 x i256> %In, %Amt
+  ret <2 x i256> %Out
+}
+
+; CHECK-LABEL: test3
+define <2 x i256> @test3(<2 x i256> %In) {
+  %Amt = insertelement <2 x i256> undef, i256 -1, i32 0
+  %Out = lshr <2 x i256> %In, %Amt
+  ret <2 x i256> %Out
+}