[DAGCombiner] Better support for shifting large value type by constants

[RKSimon]

Summary

As detailed on D22726, much of the shift combining code assume constant values will fit into a uint64_t value and calls ConstantSDNode::getZExtValue where it probably shouldn't (leading to asserts). Using APInt directly avoids this problem but we encounter other assertions if we attempt to compare/operate on 2 APInt of different bitwidths.

This patch adds a helper function to ensure that 2 APInt values are zero extended as required so that they can be safely used together. I've only added an initial example use for this to the '(SHIFT (SHIFT x, c1), c2) --> (SHIFT x, (ADD c1, c2))' combines. Further cases can easily be added as required.

[RKSimon]

ping?

[t.p.northover]

Isn't the IR already undefined whenever this triggers (unless you're working on an i18446744073709551616)?

[RKSimon]

In D23007#508614, @t.p.northover wrote:

Isn't the IR already undefined whenever this triggers (unless you're working on an i18446744073709551616)?

If the offending IR originated during dag combine then this is what should be reducing it to UNDEF. And when we are combining repeated shifts with the outer shift being valid then the inner shift might not have been reduced to UNDEF yet.

[t.p.northover]

Ah, of course. undef isn't unrestrained UB, we still have to compile it on the assumption that it's never dynamically executed.

lib/CodeGen/SelectionDAG/DAGCombiner.cpp

731–739

Lower-case 'z' for functions.

A simpler body would be:

unsigned Bits = std::max(LHS.getBitWidth(), RHS.getBitWidth());
LHS = LHS.zextOrSelf(Bits);
RHS = RHS.zextOrSelf(Bits);

4484

I don't think this is quite the right check, though it might not matter since it's UB anyway (e.g. c1 = c2 = 2^(Bits-1)). A comment that we made the decision intentionally might be useful though.

[RKSimon]

Updated based on Tim's feedback. Added better support for shift overflow.

I took the easy way out and didn't add support for i18446744073709551616 ;-)

[t.p.northover]

Thanks Simon! Looks good to me.

lib/CodeGen/SelectionDAG/DAGCombiner.cpp

static SDValue GetNegatedExpression(SDValue Op, SelectionDAG &DAG,
   case ISD::FP_ROUND:
       return DAG.getNode(ISD::FP_ROUND, SDLoc(Op), Op.getValueType(),
                          GetNegatedExpression(Op.getOperand(0), DAG,
                                               LegalOperations, Depth+1),
                          Op.getOperand(1));
   }
 }
 
+// APInts must be the same size for most operations, this helper
+// function zero extends the shorter of the pair so that they match.
+static void ZeroExtendToMatch(APInt &LHS, APInt &RHS) {
+  unsigned LHSBits = LHS.getBitWidth();
+  unsigned RHSBits = RHS.getBitWidth();
+
+  if (LHSBits > RHSBits)
+    RHS = RHS.zext(LHSBits);
+  else if (RHSBits > LHSBits)
+    LHS = LHS.zext(RHSBits);
+}
+
 // Return true if this node is a setcc, or is a select_cc
 // that selects between the target values used for true and false, making it
 // equivalent to a setcc. Also, set the incoming LHS, RHS, and CC references to
 // the appropriate nodes based on the type of node we are checking. This
 // simplifies life a bit for the callers.
 bool DAGCombiner::isSetCCEquivalent(SDValue N, SDValue &LHS, SDValue &RHS,
                                     SDValue &CC) const {
   if (N.getOpcode() == ISD::SETCC) {
SDValue DAGCombiner::visitSHL(SDNode *N) {
   }
 
   if (N1C && SimplifyDemandedBits(SDValue(N, 0)))
     return SDValue(N, 0);
 
   // fold (shl (shl x, c1), c2) -> 0 or (shl x, (add c1, c2))
   if (N1C && N0.getOpcode() == ISD::SHL) {
     if (ConstantSDNode *N0C1 = isConstOrConstSplat(N0.getOperand(1))) {
-      uint64_t c1 = N0C1->getZExtValue();
-      uint64_t c2 = N1C->getZExtValue();
+      APInt c1 = N0C1->getAPIntValue();
+      APInt c2 = N1C->getAPIntValue();
+      ZeroExtendToMatch(c1, c2);
+
       SDLoc DL(N);
-      if (c1 + c2 >= OpSizeInBits)
+      if ((c1 + c2).uge(OpSizeInBits))
         return DAG.getConstant(0, DL, VT);
       return DAG.getNode(ISD::SHL, DL, VT, N0.getOperand(0),
                          DAG.getConstant(c1 + c2, DL, N1.getValueType()));
     }
   }
 
   // fold (shl (ext (shl x, c1)), c2) -> (ext (shl x, (add c1, c2)))
   // For this to be valid, the second form must not preserve any of the bits
if (N1C && N0.getOpcode() == ISD::SHL && N1 == N0.getOperand(1)) {
     if ((!LegalOperations ||
          TLI.isOperationLegal(ISD::SIGN_EXTEND_INREG, ExtVT)))
       return DAG.getNode(ISD::SIGN_EXTEND_INREG, SDLoc(N), VT,
                          N0.getOperand(0), DAG.getValueType(ExtVT));
   }
 
   // fold (sra (sra x, c1), c2) -> (sra x, (add c1, c2))
   if (N1C && N0.getOpcode() == ISD::SRA) {
-    if (ConstantSDNode *C1 = isConstOrConstSplat(N0.getOperand(1))) {
-      unsigned Sum = N1C->getZExtValue() + C1->getZExtValue();
-      if (Sum >= OpSizeInBits)
+    if (ConstantSDNode *N0C1 = isConstOrConstSplat(N0.getOperand(1))) {
+      APInt c1 = N0C1->getAPIntValue();
+      APInt c2 = N1C->getAPIntValue();
+      ZeroExtendToMatch(c1, c2);
+
+      APInt Sum = c1 + c2;
+      if (Sum.uge(OpSizeInBits))
         Sum = OpSizeInBits - 1;
       SDLoc DL(N);
       return DAG.getNode(ISD::SRA, DL, VT, N0.getOperand(0),
                          DAG.getConstant(Sum, DL, N1.getValueType()));
     }
   }
 
   // fold (sra (shl X, m), (sub result_size, n))
if (N1C && N1C->isNullValue())
     return N0;
   // if (srl x, c) is known to be zero, return 0
   if (N1C && DAG.MaskedValueIsZero(SDValue(N, 0),
                                    APInt::getAllOnesValue(OpSizeInBits)))
     return DAG.getConstant(0, SDLoc(N), VT);
 
   // fold (srl (srl x, c1), c2) -> 0 or (srl x, (add c1, c2))
   if (N1C && N0.getOpcode() == ISD::SRL) {
-    if (ConstantSDNode *N01C = isConstOrConstSplat(N0.getOperand(1))) {
-      uint64_t c1 = N01C->getZExtValue();
-      uint64_t c2 = N1C->getZExtValue();
+    if (ConstantSDNode *N0C1 = isConstOrConstSplat(N0.getOperand(1))) {
+      APInt c1 = N0C1->getAPIntValue();
+      APInt c2 = N1C->getAPIntValue();
+      ZeroExtendToMatch(c1, c2);
+
       SDLoc DL(N);
-      if (c1 + c2 >= OpSizeInBits)
+      if ((c1 + c2).uge(OpSizeInBits))
         return DAG.getConstant(0, DL, VT);
       return DAG.getNode(ISD::SRL, DL, VT, N0.getOperand(0),
                          DAG.getConstant(c1 + c2, DL, N1.getValueType()));
     }
   }
 
   // fold (srl (trunc (srl x, c1)), c2) -> 0 or (trunc (srl x, (add c1, c2)))
   if (N1C && N0.getOpcode() == ISD::TRUNCATE &&

test/CodeGen/X86/shift-i128.ll

 }
 
 define void @test_shl_v2i128_outofrange(<2 x i128> %x, <2 x i128>* nocapture %r) nounwind {
 entry:
   %0 = shl <2 x i128> %x, <i128 -1, i128 -1>
   store <2 x i128> %0, <2 x i128>* %r, align 16
   ret void
 }
+
+define void @test_lshr_v2i128_outofrange_sum(<2 x i128> %x, <2 x i128>* nocapture %r) nounwind {
+entry:
+  %0 = lshr <2 x i128> %x, <i128 -1, i128 -1>
+  %1 = lshr <2 x i128> %0, <i128  1, i128  1>
+  store <2 x i128> %1, <2 x i128>* %r, align 16
+  ret void
+}
+
+define void @test_ashr_v2i128_outofrange_sum(<2 x i128> %x, <2 x i128>* nocapture %r) nounwind {
+entry:
+  %0 = ashr <2 x i128> %x, <i128 -1, i128 -1>
+  %1 = ashr <2 x i128> %0, <i128  1, i128  1>
+  store <2 x i128> %1, <2 x i128>* %r, align 16
+  ret void
+}
+
+define void @test_shl_v2i128_outofrange_sum(<2 x i128> %x, <2 x i128>* nocapture %r) nounwind {
+entry:
+  %0 = shl <2 x i128> %x, <i128 -1, i128 -1>
+  %1 = shl <2 x i128> %0, <i128  1, i128  1>
+  store <2 x i128> %1, <2 x i128>* %r, align 16
+  ret void
+}