PR25754: implement result legalization for UDIVREM8_ZEXT_HREG

[spatel]

Thanks for working on this!

This patch solves the bug, but I'm not sure if it's the best solution, so I'm cc'ing some people who understand this better than me to take a look too.

1. The addition to the switch in ReplaceNodeResults() doesn't have a corresponding entry for X86ISD::SDIVREM8_SEXT_HREG (signed + sext). This isn't necessary currently because PerformSExtCombine() is missing the check for an MVT::i64 that exists in PerformZExtCombine() where these nodes are created. But I think that transform should be refactored into shared code, so they stay in sync. If we add that missing type check, we can crash for the SDIVREM case too, so we should handle SDIVREM in the same way as UDIVREM.

2. I know there are ~600 lines of precedence in crash.ll for not using FileCheck on crashing bugs, but I'd prefer that we check for codegen correctness with the new test case rather than the absence of a crash. "utils/update_llc_test_checks.py" should do well with the small test case; it's probably overkill for the big unreduced cases that already exist in crash.ll though, so you may want to split the regression test into its own or some other file.

[tyomitch]

Good point that if SDIVREM8_SEXT_HREG needs not be generated for an i64 result, then UDIVREM8_ZEXT_HREG doesn't need this either.

[tyomitch]

Happy New Year everyone!

Any comments on the updated patch (from Dec 8th)?

[spatel]

I guess that works too. :)
I thought this way might lead to poorer codegen, but I'm not seeing it with the test case.
Please also add a 'TODO' comment that we should refactor the sext/zext code so that it's not duplicated.

llvm/trunk/lib/Target/X86/X86ISelLowering.cpp

if (SDValue R = WidenMaskArithmetic(N, DAG, DCI, Subtarget))
       return R;
 
   // (i8,i32 zext (udivrem (i8 x, i8 y)) ->
   // (i8,i32 (udivrem_zext_hreg (i8 x, i8 y)
   // This exposes the zext to the udivrem lowering, so that it directly extends
   // from AH (which we otherwise need to do contortions to access).
   if (N0.getOpcode() == ISD::UDIVREM &&
       N0.getResNo() == 1 && N0.getValueType() == MVT::i8 &&
-      (VT == MVT::i32 || VT == MVT::i64)) {
+      VT == MVT::i32) {
     SDVTList NodeTys = DAG.getVTList(MVT::i8, VT);
     SDValue R = DAG.getNode(X86ISD::UDIVREM8_ZEXT_HREG, dl, NodeTys,
                             N0.getOperand(0), N0.getOperand(1));
     DAG.ReplaceAllUsesOfValueWith(N0.getValue(0), R.getValue(0));
     return R.getValue(1);
   }
 
   return SDValue();
SDValue X86TargetLowering::PerformDAGCombine(SDNode *N,
   case X86ISD::FMAX:        return PerformFMinFMaxCombine(N, DAG);
   case ISD::FMINNUM:
   case ISD::FMAXNUM:        return performFMinNumFMaxNumCombine(N, DAG,
                                                                 Subtarget);
   case X86ISD::FAND:        return PerformFANDCombine(N, DAG, Subtarget);
   case X86ISD::FANDN:       return PerformFANDNCombine(N, DAG, Subtarget);
   case X86ISD::BT:          return PerformBTCombine(N, DAG, DCI);
   case X86ISD::VZEXT_MOVL:  return PerformVZEXT_MOVLCombine(N, DAG);
+// TODO: refactor the [SU]DIVREM8_[SZ]EXT_HREG code so that it's not duplicated.
   case ISD::ANY_EXTEND:
   case ISD::ZERO_EXTEND:    return PerformZExtCombine(N, DAG, DCI, Subtarget);
   case ISD::SIGN_EXTEND:    return PerformSExtCombine(N, DAG, DCI, Subtarget);
   case ISD::SIGN_EXTEND_INREG:
     return PerformSIGN_EXTEND_INREGCombine(N, DAG, Subtarget);
   case ISD::SETCC:          return PerformISDSETCCCombine(N, DAG, Subtarget);
   case X86ISD::SETCC:       return PerformSETCCCombine(N, DAG, DCI, Subtarget);
   case X86ISD::BRCOND:      return PerformBrCondCombine(N, DAG, DCI, Subtarget);

llvm/trunk/test/CodeGen/X86/divrem8_ext.ll

 ; CHECK-32: sarl $31, %edx
 ; CHECK-64: movsbq %al, %rax
 ; CHECK:    ret
   %1 = srem i8 %x, %y
   %2 = sext i8 %1 to i64
   ret i64 %2
 }
 
+define i64 @pr25754(i8 %a, i8 %c) {
+; CHECK-LABEL: pr25754
+; CHECK:    movzbl {{.+}}, %eax
+; CHECK:    divb
+; CHECK:    movzbl %ah, %ecx
+; CHECK:    movzbl %al, %eax
+; CHECK-32: addl %ecx, %eax
+; CHECK-32: sbbl %edx, %edx
+; CHECK-32: andl $1, %edx
+; CHECK-64: addq %rcx, %rax
+; CHECK:    ret
+  %r1 = urem i8 %a, %c
+  %d1 = udiv i8 %a, %c
+  %r2 = zext i8 %r1 to i64
+  %d2 = zext i8 %d1 to i64
+  %ret = add i64 %r2, %d2
+  ret i64 %ret
+}
+
 @z = external global i8