[X86] Improve handling of UDIVREM8_ZEXT_HREG/SDIVREM8_SEXT_HREG to support 64-bit extensions.

[craig.topper]

Summary

The code for handling UDIVREM8_ZEXT_HREG with a 64-bit result type has been dead since r256924.

Though I'm not entirely sure it should be dead. The output from the test case from that revision shows an extra movzx that we could get rid of if this code was live. So I'm posting this patch to start a conversation

define i64 @pr25754(i8 %a, i8 %c) {
; X32-LABEL: pr25754:
; X32:       # BB#0:
; X32-NEXT:    movzbl {{[0-9]+}}(%esp), %eax
; X32-NEXT:    # kill: %EAX<def> %EAX<kill> %AX<def>
; X32-NEXT:    divb {{[0-9]+}}(%esp)
; X32-NEXT:    movzbl %ah, %ecx
; X32-NEXT:    movzbl %al, %eax
; X32-NEXT:    addl %ecx, %eax
; X32-NEXT:    xorl %edx, %edx
; X32-NEXT:    retl
;
; X64-LABEL: pr25754:
; X64:       # BB#0:
; X64-NEXT:    movzbl %dil, %eax
; X64-NEXT:    # kill: %EAX<def> %EAX<kill> %AX<def>
; X64-NEXT:    divb %sil
; X64-NEXT:    movzbl %ah, %ecx
; X64-NEXT:    movzbl %cl, %ecx
; X64-NEXT:    movzbl %al, %eax
; X64-NEXT:    addq %rcx, %rax
; X64-NEXT:    retq
  %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
}

[RKSimon]

Is this a missing simpifydemandedbits pattern?

[craig.topper]

If the extend type is 64-bits, emit a 32-bit -> 64-bit extend after the UDIVREM8_ZEXT_HREG/UDIVREM8_SEXT_HREG operation.

This gives a shorter encoding for the second extend in the sext case, and allows us to completely remove the second extend in the zext case.

[craig.topper]

Ping

[craig.topper]

Ping

[craig.topper]

Add known and sign bits support for UDIVREM8_ZEXT_HREG/SDIVREM8_SEXT_HREG. The UDIVREM8_ZEXT_HREG is needed to prevent a regression on one of the test cases where an extra setb was being generated.

[RKSimon]

LGTM - possibly split the sign/known bits change from the X86ISelDAGToDAG.cpp change but its not vital to do so

lib/Target/X86/X86ISelDAGToDAG.cpp

if (HiReg == X86::AH && !SDValue(Node, 1).use_empty()) {
 
       SDNode *RNode = CurDAG->getMachineNode(AHExtOpcode, dl, MVT::i32,
                                              MVT::Glue, AHCopy, InFlag);
       SDValue Result(RNode, 0);
       InFlag = SDValue(RNode, 1);
 
       if (Opcode == X86ISD::UDIVREM8_ZEXT_HREG ||
           Opcode == X86ISD::SDIVREM8_SEXT_HREG) {
-        if (Node->getValueType(1) == MVT::i64) {
-          // It's not possible to directly movsx AH to a 64bit register, because
-          // the latter needs the REX prefix, but the former can't have it.
-          assert(Opcode != X86ISD::SDIVREM8_SEXT_HREG &&
-                 "Unexpected i64 sext of h-register");
-          Result =
-              SDValue(CurDAG->getMachineNode(
-                          TargetOpcode::SUBREG_TO_REG, dl, MVT::i64,
-                          CurDAG->getTargetConstant(0, dl, MVT::i64), Result,
-                          CurDAG->getTargetConstant(X86::sub_32bit, dl,
-                                                    MVT::i32)),
-                      0);
-        }
+        assert(Node->getValueType(1) == MVT::i32 && "Unexpected result type!");
       } else {
         Result =
             CurDAG->getTargetExtractSubreg(X86::sub_8bit, dl, MVT::i8, Result);
       }
       ReplaceUses(SDValue(Node, 1), Result);
       DEBUG(dbgs() << "=> "; Result.getNode()->dump(CurDAG); dbgs() << '\n');
     }
     // Copy the division (low) result, if it is needed.

lib/Target/X86/X86ISelLowering.cpp

case X86ISD::CMOV: {
     KnownBits Known2;
     DAG.computeKnownBits(Op.getOperand(0), Known2, Depth+1);
 
     // Only known if known in both the LHS and RHS.
     Known.One &= Known2.One;
     Known.Zero &= Known2.Zero;
     break;
   }
+  case X86ISD::UDIVREM8_ZEXT_HREG:
+    // TODO: Support more than just the zero extended bits?
+    if (Op.getResNo() != 1)
+      break;
+    // The remainder is zero extended.
+    Known.Zero.setBitsFrom(8);
+    break;
   }
 }
 
 unsigned X86TargetLowering::ComputeNumSignBitsForTargetNode(
     SDValue Op, const APInt &DemandedElts, const SelectionDAG &DAG,
     unsigned Depth) const {
   unsigned VTBits = Op.getScalarValueSizeInBits();
   unsigned Opcode = Op.getOpcode();
case X86ISD::VPCOMU:
     return VTBits;
 
   case X86ISD::CMOV: {
     unsigned Tmp0 = DAG.ComputeNumSignBits(Op.getOperand(0), Depth+1);
     if (Tmp0 == 1) return 1;  // Early out.
     unsigned Tmp1 = DAG.ComputeNumSignBits(Op.getOperand(1), Depth+1);
     return std::min(Tmp0, Tmp1);
   }
+  case X86ISD::SDIVREM8_SEXT_HREG:
+    // TODO: Support more than just the sign extended bits?
+    if (Op.getResNo() != 1)
+      break;
+    // The remainder is sign extended.
+    return VTBits - 7;
   }
 
   // Fallback case.
   return 1;
 }
 
 SDValue X86TargetLowering::unwrapAddress(SDValue N) const {
   if (N->getOpcode() == X86ISD::Wrapper || N->getOpcode() == X86ISD::WrapperRIP)
static SDValue getDivRem8(SDNode *N, SelectionDAG &DAG) {
   auto OpcodeN = N->getOpcode();
   auto OpcodeN0 = N0.getOpcode();
   if (!((OpcodeN == ISD::SIGN_EXTEND && OpcodeN0 == ISD::SDIVREM) ||
         (OpcodeN == ISD::ZERO_EXTEND && OpcodeN0 == ISD::UDIVREM)))
     return SDValue();
 
   EVT VT = N->getValueType(0);
   EVT InVT = N0.getValueType();
-  if (N0.getResNo() != 1 || InVT != MVT::i8 || VT != MVT::i32)
+  if (N0.getResNo() != 1 || InVT != MVT::i8 ||
+      !(VT == MVT::i32 || VT == MVT::i64))
     return SDValue();
 
-  SDVTList NodeTys = DAG.getVTList(MVT::i8, VT);
+  SDVTList NodeTys = DAG.getVTList(MVT::i8, MVT::i32);
   auto DivRemOpcode = OpcodeN0 == ISD::SDIVREM ? X86ISD::SDIVREM8_SEXT_HREG
                                                : X86ISD::UDIVREM8_ZEXT_HREG;
   SDValue R = DAG.getNode(DivRemOpcode, SDLoc(N), NodeTys, N0.getOperand(0),
                           N0.getOperand(1));
   DAG.ReplaceAllUsesOfValueWith(N0.getValue(0), R.getValue(0));
+  // If this was a 64-bit extend, complete it.
+  if (VT == MVT::i64)
+    return DAG.getNode(OpcodeN, SDLoc(N), VT, R.getValue(1));
   return R.getValue(1);
 }
 
 // If we face {ANY,SIGN,ZERO}_EXTEND that is applied to a CMOV with constant
 // operands and the result of CMOV is not used anywhere else - promote CMOV
 // itself instead of promoting its result. This could be beneficial, because:
 //     1) X86TargetLowering::EmitLoweredSelect later can do merging of two
 //        (or more) pseudo-CMOVs only when they go one-after-another and

test/CodeGen/X86/divrem8_ext.ll

 ; X32-NEXT:    retl
 ;
 ; X64-LABEL: test_urem_zext64_ah:
 ; X64:       # BB#0:
 ; X64-NEXT:    movzbl %dil, %eax
 ; X64-NEXT:    # kill: %EAX<def> %EAX<kill> %AX<def>
 ; X64-NEXT:    divb %sil
 ; X64-NEXT:    movzbl %ah, %eax # NOREX
-; X64-NEXT:    movzbl %al, %eax
 ; X64-NEXT:    retq
   %1 = urem i8 %x, %y
   %2 = zext i8 %1 to i64
   ret i64 %2
 }
 
 define signext i8 @test_sdivrem_sext_ah(i8 %x, i8 %y) {
 ; X32-LABEL: test_sdivrem_sext_ah:

 ; X32-NEXT:    retl
 ;
 ; X64-LABEL: test_srem_sext64_ah:
 ; X64:       # BB#0:
 ; X64-NEXT:    movl %edi, %eax
 ; X64-NEXT:    cbtw
 ; X64-NEXT:    idivb %sil
 ; X64-NEXT:    movsbl %ah, %eax # NOREX
-; X64-NEXT:    movsbq %al, %rax
+; X64-NEXT:    cltq
 ; X64-NEXT:    retq
   %1 = srem i8 %x, %y
   %2 = sext i8 %1 to i64
   ret i64 %2
 }
 
 define i64 @pr25754(i8 %a, i8 %c) {
 ; X32-LABEL: pr25754:
 ; X32:       # BB#0:
 ; X32-NEXT:    movzbl {{[0-9]+}}(%esp), %eax
 ; X32-NEXT:    # kill: %EAX<def> %EAX<kill> %AX<def>
 ; X32-NEXT:    divb {{[0-9]+}}(%esp)
 ; X32-NEXT:    movzbl %ah, %ecx # NOREX
 ; X32-NEXT:    movzbl %al, %eax
 ; X32-NEXT:    addl %ecx, %eax
 ; X32-NEXT:    xorl %edx, %edx
 ; X32-NEXT:    retl
 ;
 ; X64-LABEL: pr25754:
 ; X64:       # BB#0:
 ; X64-NEXT:    movzbl %dil, %eax
 ; X64-NEXT:    # kill: %EAX<def> %EAX<kill> %AX<def>
 ; X64-NEXT:    divb %sil
 ; X64-NEXT:    movzbl %ah, %ecx # NOREX
-; X64-NEXT:    movzbl %cl, %ecx
 ; X64-NEXT:    movzbl %al, %eax
 ; X64-NEXT:    addq %rcx, %rax
 ; X64-NEXT:    retq
   %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