Fix llc crash processing S/UREM for -Oz builds caused by rL250825.

[srking]

Summary

When taking the remainder of a value divided by a constant, visitREM() attempts to convert the REM to a longer but faster sequence of instructions. This conversion calls combine() on a speculative DIV instruction. Commit rL250825 may cause this combine() to return a DIVREM, corrupting nearby nodes. Flow eventually hits unreachable().

This patch adds a test case and a check to prevent visitREM() from trying to convert the REM instruction in cases where a DIVREM is possible.

[arsenm]

test/CodeGen/Generic/urem_crash.ll

1 ↗

(On Diff #38285)

This definitely needs to check something and should not be under CodeGen/Generic

[srking]

test/CodeGen/Generic/urem_crash.ll

1 ↗

(On Diff #38285)

Where should the test live?
The simple no-check style was keeping with another "crash" test I used for reference.

[arsenm]

test/CodeGen/Generic/urem_crash.ll

1 ↗

(On Diff #38285)

There should probably be a test for a target with and without isIntDivCheap.

[srking]

test/CodeGen/Generic/urem_crash.ll

1 ↗

(On Diff #38285)

Sounds good. x86 would work. I'll update patch.

[srking]

Moved test to X86 directory and added new test functions without minsize attribute. X86 is a better home for the test since X86 returns different values for isIntDivCheap() depending on the minsize attribute.
Verified the test catches the crash bug when the s/urem fix is not in place.
Verified all tests pass with the fix.

By design, the test does not care how rem and div are eventually lowered, only that lowering does not cause node corruption.

[tyomitch]

lib/CodeGen/SelectionDAG/DAGCombiner.cpp

2332

I don't understand this comment. In particular, the code following the comment doesn't check for isIntDivCheap() before returning DIVREM. Is this intended as a FIXME: ?

[srking]

lib/CodeGen/SelectionDAG/DAGCombiner.cpp

2332

This comment is a warning, not a FIXME. This patch depends on the *current* way isIntDivCheap() checking happens in visitDIV() to prevent visitDIV() from returning a DIVREM when the caller, visitREM() in this case, can't handle such transformation. The nature and reason for this dependency is the brief text of the comment.

[srking]

Improved comment and added check in visitSDIV and visitUDIV in response to feedback from Artyom Skrobov.

[tyomitch]

Thanks! The new comments are much more clear.

[srking]

Commit r251373.

lib/CodeGen/SelectionDAG/DAGCombiner.cpp

SDValue DAGCombiner::visitSDIV(SDNode *N) {
   // alternate sequence.  Targets may check function attributes for size/speed
   // trade-offs.
   AttributeSet Attr = DAG.getMachineFunction().getFunction()->getAttributes();
   if (N1C && !TLI.isIntDivCheap(N->getValueType(0), Attr))
     if (SDValue Op = BuildSDIV(N))
       return Op;
 
   // sdiv, srem -> sdivrem
+  // If the divisor is constant, then return DIVREM only if isIntDivCheap() is true.
+  // Otherwise, we break the simplification logic in visitREM().
+  if (!N1C || TLI.isIntDivCheap(N->getValueType(0), Attr))
     if (SDValue DivRem = useDivRem(N))
         return DivRem;
 
   // undef / X -> 0
   if (N0.getOpcode() == ISD::UNDEF)
     return DAG.getConstant(0, DL, VT);
   // X / undef -> undef
   if (N1.getOpcode() == ISD::UNDEF)
     return N1;
 
SDValue DAGCombiner::visitUDIV(SDNode *N) {
 
   // fold (udiv x, c) -> alternate
   AttributeSet Attr = DAG.getMachineFunction().getFunction()->getAttributes();
   if (N1C && !TLI.isIntDivCheap(N->getValueType(0), Attr))
     if (SDValue Op = BuildUDIV(N))
       return Op;
 
   // sdiv, srem -> sdivrem
+  // If the divisor is constant, then return DIVREM only if isIntDivCheap() is true.
+  // Otherwise, we break the simplification logic in visitREM().
+  if (!N1C || TLI.isIntDivCheap(N->getValueType(0), Attr))
     if (SDValue DivRem = useDivRem(N))
         return DivRem;
 
   // undef / X -> 0
   if (N0.getOpcode() == ISD::UNDEF)
     return DAG.getConstant(0, DL, VT);
   // X / undef -> undef
   if (N1.getOpcode() == ISD::UNDEF)
     return N1;
 
if (N1.getOpcode() == ISD::SHL) {
                                  VT));
           AddToWorklist(Add.getNode());
           return DAG.getNode(ISD::AND, DL, VT, N0, Add);
         }
       }
     }
   }
 
+  AttributeSet Attr = DAG.getMachineFunction().getFunction()->getAttributes();
+
   // If X/C can be simplified by the division-by-constant logic, lower
   // X%C to the equivalent of X-X/C*C.
-  if (N1C && !N1C->isNullValue()) {
+  // To avoid mangling nodes, this simplification requires that the combine()
+  // call for the speculative DIV must not cause a DIVREM conversion.  We guard
+  // against this by skipping the simplification if isIntDivCheap().  When
+  // div is not cheap, combine will not return a DIVREM.  Regardless,
+  // checking cheapness here makes sense since the simplification results in
+  // fatter code.
+  if (N1C && !N1C->isNullValue() && !TLI.isIntDivCheap(VT, Attr)) {
     unsigned DivOpcode = isSigned ? ISD::SDIV : ISD::UDIV;
     SDValue Div = DAG.getNode(DivOpcode, DL, VT, N0, N1);
     AddToWorklist(Div.getNode());
     SDValue OptimizedDiv = combine(Div.getNode());
     if (OptimizedDiv.getNode() && OptimizedDiv.getNode() != Div.getNode()) {
+      assert((OptimizedDiv.getOpcode() != ISD::UDIVREM) &&
+             (OptimizedDiv.getOpcode() != ISD::SDIVREM));
       SDValue Mul = DAG.getNode(ISD::MUL, DL, VT, OptimizedDiv, N1);
       SDValue Sub = DAG.getNode(ISD::SUB, DL, VT, N0, Mul);
       AddToWorklist(Mul.getNode());
       return Sub;
     }
   }
 
   // sdiv, srem -> sdivrem

test/CodeGen/X86/rem_crash.ll

+; RUN: llc < %s
+
+define i8 @test_minsize_uu8(i8 %x) minsize optsize {
+entry:
+  %0 = udiv i8 %x, 10
+  %1 = urem i8 %x, 10
+  %res = add i8 %0, %1
+  ret i8 %res
+}
+
+define i8 @test_minsize_ss8(i8 %x) minsize optsize {
+entry:
+  %0 = sdiv i8 %x, 10
+  %1 = srem i8 %x, 10
+  %res = add i8 %0, %1
+  ret i8 %res
+}
+
+define i8 @test_minsize_us8(i8 %x) minsize optsize {
+entry:
+  %0 = udiv i8 %x, 10
+  %1 = srem i8 %x, 10
+  %res = add i8 %0, %1
+  ret i8 %res
+}
+
+define i8 @test_minsize_su8(i8 %x) minsize optsize {
+entry:
+  %0 = sdiv i8 %x, 10
+  %1 = urem i8 %x, 10
+  %res = add i8 %0, %1
+  ret i8 %res
+}
+
+define i16 @test_minsize_uu16(i16 %x) minsize optsize {
+entry:
+  %0 = udiv i16 %x, 10
+  %1 = urem i16 %x, 10
+  %res = add i16 %0, %1
+  ret i16 %res
+}
+
+define i16 @test_minsize_ss16(i16 %x) minsize optsize {
+entry:
+  %0 = sdiv i16 %x, 10
+  %1 = srem i16 %x, 10
+  %res = add i16 %0, %1
+  ret i16 %res
+}
+
+define i16 @test_minsize_us16(i16 %x) minsize optsize {
+entry:
+  %0 = udiv i16 %x, 10
+  %1 = srem i16 %x, 10
+  %res = add i16 %0, %1
+  ret i16 %res
+}
+
+define i16 @test_minsize_su16(i16 %x) minsize optsize {
+entry:
+  %0 = sdiv i16 %x, 10
+  %1 = urem i16 %x, 10
+  %res = add i16 %0, %1
+  ret i16 %res
+}
+
+define i32 @test_minsize_uu32(i32 %x) minsize optsize {
+entry:
+  %0 = udiv i32 %x, 10
+  %1 = urem i32 %x, 10
+  %res = add i32 %0, %1
+  ret i32 %res
+}
+
+define i32 @test_minsize_ss32(i32 %x) minsize optsize {
+entry:
+  %0 = sdiv i32 %x, 10
+  %1 = srem i32 %x, 10
+  %res = add i32 %0, %1
+  ret i32 %res
+}
+
+define i32 @test_minsize_us32(i32 %x) minsize optsize {
+entry:
+  %0 = udiv i32 %x, 10
+  %1 = srem i32 %x, 10
+  %res = add i32 %0, %1
+  ret i32 %res
+}
+
+define i32 @test_minsize_su32(i32 %x) minsize optsize {
+entry:
+  %0 = sdiv i32 %x, 10
+  %1 = urem i32 %x, 10
+  %res = add i32 %0, %1
+  ret i32 %res
+}
+
+define i64 @test_minsize_uu64(i64 %x) minsize optsize {
+entry:
+  %0 = udiv i64 %x, 10
+  %1 = urem i64 %x, 10
+  %res = add i64 %0, %1
+  ret i64 %res
+}
+
+define i64 @test_minsize_ss64(i64 %x) minsize optsize {
+entry:
+  %0 = sdiv i64 %x, 10
+  %1 = srem i64 %x, 10
+  %res = add i64 %0, %1
+  ret i64 %res
+}
+
+define i64 @test_minsize_us64(i64 %x) minsize optsize {
+entry:
+  %0 = udiv i64 %x, 10
+  %1 = srem i64 %x, 10
+  %res = add i64 %0, %1
+  ret i64 %res
+}
+
+define i64 @test_minsize_su64(i64 %x) minsize optsize {
+entry:
+  %0 = sdiv i64 %x, 10
+  %1 = urem i64 %x, 10
+  %res = add i64 %0, %1
+  ret i64 %res
+}
+
+define i8 @test_uu8(i8 %x) optsize {
+entry:
+  %0 = udiv i8 %x, 10
+  %1 = urem i8 %x, 10
+  %res = add i8 %0, %1
+  ret i8 %res
+}
+
+define i8 @test_ss8(i8 %x) optsize {
+entry:
+  %0 = sdiv i8 %x, 10
+  %1 = srem i8 %x, 10
+  %res = add i8 %0, %1
+  ret i8 %res
+}
+
+define i8 @test_us8(i8 %x) optsize {
+entry:
+  %0 = udiv i8 %x, 10
+  %1 = srem i8 %x, 10
+  %res = add i8 %0, %1
+  ret i8 %res
+}
+
+define i8 @test_su8(i8 %x) optsize {
+entry:
+  %0 = sdiv i8 %x, 10
+  %1 = urem i8 %x, 10
+  %res = add i8 %0, %1
+  ret i8 %res
+}
+
+define i16 @test_uu16(i16 %x) optsize {
+entry:
+  %0 = udiv i16 %x, 10
+  %1 = urem i16 %x, 10
+  %res = add i16 %0, %1
+  ret i16 %res
+}
+
+define i16 @test_ss16(i16 %x) optsize {
+entry:
+  %0 = sdiv i16 %x, 10
+  %1 = srem i16 %x, 10
+  %res = add i16 %0, %1
+  ret i16 %res
+}
+
+define i16 @test_us16(i16 %x) optsize {
+entry:
+  %0 = udiv i16 %x, 10
+  %1 = srem i16 %x, 10
+  %res = add i16 %0, %1
+  ret i16 %res
+}
+
+define i16 @test_su16(i16 %x) optsize {
+entry:
+  %0 = sdiv i16 %x, 10
+  %1 = urem i16 %x, 10
+  %res = add i16 %0, %1
+  ret i16 %res
+}
+
+define i32 @test_uu32(i32 %x) optsize {
+entry:
+  %0 = udiv i32 %x, 10
+  %1 = urem i32 %x, 10
+  %res = add i32 %0, %1
+  ret i32 %res
+}
+
+define i32 @test_ss32(i32 %x) optsize {
+entry:
+  %0 = sdiv i32 %x, 10
+  %1 = srem i32 %x, 10
+  %res = add i32 %0, %1
+  ret i32 %res
+}
+
+define i32 @test_us32(i32 %x) optsize {
+entry:
+  %0 = udiv i32 %x, 10
+  %1 = srem i32 %x, 10
+  %res = add i32 %0, %1
+  ret i32 %res
+}
+
+define i32 @test_su32(i32 %x) optsize {
+entry:
+  %0 = sdiv i32 %x, 10
+  %1 = urem i32 %x, 10
+  %res = add i32 %0, %1
+  ret i32 %res
+}
+
+define i64 @test_uu64(i64 %x) optsize {
+entry:
+  %0 = udiv i64 %x, 10
+  %1 = urem i64 %x, 10
+  %res = add i64 %0, %1
+  ret i64 %res
+}
+
+define i64 @test_ss64(i64 %x) optsize {
+entry:
+  %0 = sdiv i64 %x, 10
+  %1 = srem i64 %x, 10
+  %res = add i64 %0, %1
+  ret i64 %res
+}
+
+define i64 @test_us64(i64 %x) optsize {
+entry:
+  %0 = udiv i64 %x, 10
+  %1 = srem i64 %x, 10
+  %res = add i64 %0, %1
+  ret i64 %res
+}
+
+define i64 @test_su64(i64 %x) optsize {
+entry:
+  %0 = sdiv i64 %x, 10
+  %1 = urem i64 %x, 10
+  %res = add i64 %0, %1
+  ret i64 %res
+}