[DAGCombiner][X86] Fold (sub (subcarry X, 0, Carry), Y) -> (subcarry X, Y, Carry)

[craig.topper]

Summary

Fixes PR57576.

[spatel]

Is the transform still profitable if the subcarry has another use? (not sure how to write a test for that)

In IR, it looks like we'd reduce the equivalent of "subcarry X, 0, Carry" to "usubo X, Carry". Are those the same semantics for this node? And would that simpler transform make sense in SDAG too?

[craig.topper]

In D133471#3777069, @spatel wrote:

Is the transform still profitable if the subcarry has another use? (not sure how to write a test for that)

I copied this code from visitAddLikeCommutative and changed ADDCARRY to SUBCARRY. So any one uses check is also missing there.

In IR, it looks like we'd reduce the equivalent of "subcarry X, 0, Carry" to "usubo X, Carry". Are those the same semantics for this node? And would that simpler transform make sense in SDAG too?

Carry here has bool type so I don't think we can fold it to "usubo X, Carry" without an extend.

[spatel]

In D133471#3777372, @craig.topper wrote:

In D133471#3777069, @spatel wrote:

Is the transform still profitable if the subcarry has another use? (not sure how to write a test for that)

I copied this code from visitAddLikeCommutative and changed ADDCARRY to SUBCARRY. So any one uses check is also missing there.

I see...that goes back to D32738. I'd put the one-use limit on both as a conservative first step unless there's a test showing this way is better.

LGTM, but let's see if @deadalnix has any suggestions.

llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp

SDValue DAGCombiner::visitSUB(SDNode *N) {
   // Subtracting SMIN/0 is the same as adding SMIN/0:
   // N0 - (X << BW-1) --> N0 + (X << BW-1)
   if (N1.getOpcode() == ISD::SHL) {
     ConstantSDNode *ShlC = isConstOrConstSplat(N1.getOperand(1));
     if (ShlC && ShlC->getAPIntValue() == VT.getScalarSizeInBits() - 1)
       return DAG.getNode(ISD::ADD, DL, VT, N1, N0);
   }
 
+  // (sub (subcarry X, 0, Carry), Y) -> (subcarry X, Y, Carry)
+  if (N0.getOpcode() == ISD::SUBCARRY && isNullConstant(N0.getOperand(1)) &&
+      N0.getResNo() == 0 && N0.hasOneUse())
+    return DAG.getNode(ISD::SUBCARRY, DL, N0->getVTList(),
+                       N0.getOperand(0), N1, N0.getOperand(2));
+
   if (TLI.isOperationLegalOrCustom(ISD::ADDCARRY, VT)) {
     // (sub Carry, X)  ->  (addcarry (sub 0, X), 0, Carry)
     if (SDValue Carry = getAsCarry(TLI, N0)) {
       SDValue X = N1;
       SDValue Zero = DAG.getConstant(0, DL, VT);
       SDValue NegX = DAG.getNode(ISD::SUB, DL, VT, Zero, X);
       return DAG.getNode(ISD::ADDCARRY, DL,
                          DAG.getVTList(VT, Carry.getValueType()), NegX, Zero,

llvm/test/CodeGen/X86/pr57576.ll

 ; NOTE: Assertions have been autogenerated by utils/update_llc_test_checks.py
 ; RUN: llc < %s -mtriple=x86_64-unknown-linux-gnu | FileCheck %s
 
 define { i64, i64 } @sub(i64 noundef %0, i64 noundef %1, i64 noundef %2, i64 noundef %3) {
 ; CHECK-LABEL: sub:
 ; CHECK:       # %bb.0:
 ; CHECK-NEXT:    movq %rdi, %rax
 ; CHECK-NEXT:    subq %rdx, %rax
-; CHECK-NEXT:    sbbq $0, %rsi
-; CHECK-NEXT:    subq %rcx, %rsi
+; CHECK-NEXT:    sbbq %rcx, %rsi
 ; CHECK-NEXT:    movq %rsi, %rdx
 ; CHECK-NEXT:    retq
   %5 = zext i64 %1 to i128
   %6 = shl nuw i128 %5, 64
   %7 = zext i64 %0 to i128
   %8 = zext i64 %3 to i128
   %9 = mul i128 %8, -18446744073709551616
   %10 = zext i64 %2 to i128