[SelectionDAG] Fix a crash visiting `AND` nodes

[davide]

Summary

When visiting we crash in t16: i64 = srl t40, Constant:i8<0> because we don't expect a shift-by-zero in visitAndLike.
I don't see a reason why this is invalid, and as discussed with Matt on IRC (which originally introduced this assertion) is probably fine to relax the condition a bit.

SelectionDAG has 11 nodes:
  t0: ch = EntryToken
  t2: i64 = Constant<0>
  t29: i32,ch = load<LD2[undef](align=8), sext from i16> t0, undef:i64, undef:i64
    t19: ch = TokenFactor t29:1, t0
        t40: i64 = sign_extend t29
      t16: i64 = srl t40, Constant:i8<0>
    t24: i64 = and t16, Constant:i64<65535>
  t20: ch = store<ST8[undef]> t19, t24, undef:i64, undef:i64

[escha]

Might it be better to just bail if ShiftBits is zero? This seems like a classic case of "trying to optimize a node that itself is going to disappear when it gets combine()'d", so maybe we should just not do this "optimization" if the shift is no shift at all.

[hfinkel]

In D26098#582458, @escha wrote:

Might it be better to just bail if ShiftBits is zero? This seems like a classic case of "trying to optimize a node that itself is going to disappear when it gets combine()'d", so maybe we should just not do this "optimization" if the shift is no shift at all.

I agree. Performing this transformation won't make it any easier to get rid of the zero shift (and if it does, it shouldn't).

[davide]

In D26098#582458, @escha wrote:

Might it be better to just bail if ShiftBits is zero? This seems like a classic case of "trying to optimize a node that itself is going to disappear when it gets combine()'d", so maybe we should just not do this "optimization" if the shift is no shift at all.

Sounds reasonable.

[spatel]

test/CodeGen/X86/visitand-shift.ll

1 ↗

(On Diff #76244)

We shouldn't need the "-O0" here?
Also, please give the file and/or the test a meaningful name - PR30813?

[davide]

Update correct revision.

test/CodeGen/X86/visitand-shift.ll

1 ↗

(On Diff #76244)

Renamed the file. It doesn't crash without -O0.

[hfinkel]

lib/CodeGen/SelectionDAG/DAGCombiner.cpp

3053

To micro-optimize, I'd move this to before we count bits in the AndMask and get the integer VT. Otherwise, LGTM.

lib/CodeGen/SelectionDAG/DAGCombiner.cpp

     if (ConstantSDNode *CAnd = dyn_cast<ConstantSDNode>(N1)) {
       if (ConstantSDNode *CShift = dyn_cast<ConstantSDNode>(N0.getOperand(1))) {
         unsigned Size = VT.getSizeInBits();
         const APInt &AndMask = CAnd->getAPIntValue();
         unsigned ShiftBits = CShift->getZExtValue();
         unsigned MaskBits = AndMask.countTrailingOnes();
         EVT HalfVT = EVT::getIntegerVT(*DAG.getContext(), Size / 2);
 
+        // Bail out, this node will probably disappear anyway.
+        if (ShiftBits == 0)
+          return SDValue();
+
         if (APIntOps::isMask(AndMask) &&
             // Required bits must not span the two halves of the integer and
             // must fit in the half size type.
             (ShiftBits + MaskBits <= Size / 2) &&
             TLI.isNarrowingProfitable(VT, HalfVT) &&
             TLI.isTypeDesirableForOp(ISD::AND, HalfVT) &&
             TLI.isTypeDesirableForOp(ISD::SRL, HalfVT) &&
             TLI.isTruncateFree(VT, HalfVT) &&
             TLI.isZExtFree(HalfVT, VT)) {
           // The isNarrowingProfitable is to avoid regressions on PPC and
           // AArch64 which match a few 64-bit bit insert / bit extract patterns
           // on downstream users of this. Those patterns could probably be
           // extended to handle extensions mixed in.
 
           SDValue SL(N0);
-          assert(ShiftBits != 0 && MaskBits <= Size);
+          assert(MaskBits <= Size);
 
           // Extracting the highest bit of the low half.
           EVT ShiftVT = TLI.getShiftAmountTy(HalfVT, DAG.getDataLayout());
           SDValue Trunc = DAG.getNode(ISD::TRUNCATE, SL, HalfVT,
                                       N0.getOperand(0));
 
           SDValue NewMask = DAG.getConstant(AndMask.trunc(Size / 2), SL, HalfVT);
           SDValue ShiftK = DAG.getConstant(ShiftBits, SL, ShiftVT);

test/CodeGen/X86/pr30813.ll

+; RUN: llc -O0 %s -o - | FileCheck %s
+; CHECK: patatino:
+; CHECK:         .cfi_startproc
+; CHECK:         movzwl  (%rax), %ecx
+; CHECK:         movl    %ecx, %eax
+; CHECK:         movq    %rax, (%rdx)
+; CHECK:         retq
+
+define void @patatino() {
+  %tmp = load i16, i16* undef, align 8
+  %conv18098 = sext i16 %tmp to i64
+  %and1 = and i64 %conv18098, -1
+  %cmp = icmp ult i64 -1, undef
+  %conv = sext i1 %cmp to i64
+  %load1 = load i48, i48* undef, align 8
+  %bf.cast18158 = sext i48 %load1 to i64
+  %conv18159 = trunc i64 %bf.cast18158 to i32
+  %conv18160 = sext i32 %conv18159 to i64
+  %div18162 = udiv i64 %conv, %conv18160
+  %and18163 = and i64 %conv18098, %div18162
+  %shr18164 = lshr i64 %and1, %and18163
+  %conv18165 = trunc i64 %shr18164 to i16
+  %conv18166 = zext i16 %conv18165 to i64
+  store i64 %conv18166, i64* undef, align 8
+  store i48 undef, i48* undef, align 8
+  ret void
+}