[DAGCombiner][X86][SystemZ] Canonicalize SSUBO with immediate RHS to SADDO by negating the immediate.

[craig.topper]

Summary

This lines up with what we do for regular subtract and it matches up better with X86 assumptions in isel patterns that add with immediate is more canonical than sub with immediate.

[craig.topper]

llvm/test/CodeGen/X86/sub-with-overflow.ll

86–87

This is somewhat a regression, but we now generate the code we would get from sadd.with.overflow.i32 with an immediate of -1. So we probably need a more complete canonicalization to icmp when the data result isn't used.

[RKSimon]

llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp

3046

(subo x, c)

3049

Might be wrong here, but what happens with (subo x, INT_MIN) ? That can't be expressed as a +ve integer and we're dealing with overflows here....

[spatel]

cc @lebedev.ri @nikic @dlrobertson - do we have a patch proposal for this transform in IR? (I'm not finding it now.)

[craig.topper]

llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp

3049

I think you're right. This generates the opposite value for the overflow flag for INT_MIN.

[nikic]

In D60020#1449271, @spatel wrote:

cc @lebedev.ri @nikic @dlrobertson - do we have a patch proposal for this transform in IR? (I'm not finding it now.)

Don't think we have a patch open for this yet, but @dlrobertson added some tests for this transform in D59653 already, so probably the instcombine patch will come as well...

[dlrobertson]

In D60020#1449375, @nikic wrote:

In D60020#1449271, @spatel wrote:

cc @lebedev.ri @nikic @dlrobertson - do we have a patch proposal for this transform in IR? (I'm not finding it now.)

Don't think we have a patch open for this yet, but @dlrobertson added some tests for this transform in D59653 already, so probably the instcombine patch will come as well...

Sorry for the delay. Was planning on posting a patch for the IR transform in the next day or so.

[spatel]

D60061 is the sibling patch for IR. Does that make the backend patch unnecessary? If not, then let's make sure the behavior is consistent between these patches.

[craig.topper]

-Add a check for INT_MIN.
-Update a SystemZ test I missed cause I only had X86 building previously. I think this one of these cases is a regression, but that's probably also going to happen with the InstCombine version of this patch too. So probably needs to be fixed regardless.

[dlrobertson]

llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp

3047

I don't think this path would be hit post-D60061 right? Would this be for the case that opt is run without instcombine?

Also D60061 uses Constant and isNotMinSignedValue. Is there a reason not to use N1C->getConstantIntValue()->isNotMinSignedValue here too? Are there pros/cons to either one?

[craig.topper]

llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp

3047

Not sure if this can come up in real code after D60061, but we have lit tests here that change. Should we update those to post D60061 code instead?

IsNotMinSignedValue does dyn_cast checks for ConstantInt, ConstantFP, and vectors. Seems like a bit of wasted work if we already know we're looking at an integer.

[nikic]

llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp

3047

ssubo nodes may be created during ssubsat legalization, so it's possible for nodes of this form to occur during codegen of canonical IR, even after D60061. With that in mind, I think it's okay to have this as a DAG transform as well.

isNotMinSignedValue() isn't applicable here as it operates on IR constants. We could replicate the same effect using matchUnaryPredicate() here, if we're interested in parity.

[uweigand]

In D60020#1452183, @craig.topper wrote:

-Update a SystemZ test I missed cause I only had X86 building previously. I think this one of these cases is a regression, but that's probably also going to happen with the InstCombine version of this patch too. So probably needs to be fixed regardless.

Indeed. In fact, we see the suboptimal code today when compiling a sadd.with.overflow with the negated constant, so this should be fixed anyway. I've checked the fix in as rev 357597.

If you rebase against that revision, you should no longer see the regression.

[craig.topper]

Rebase

[lebedev.ri]

LG

llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp

if (!N->hasAnyUseOfValue(1))
     return CombineTo(N, DAG.getNode(ISD::SUB, DL, VT, N0, N1),
                      DAG.getUNDEF(CarryVT));
 
   // fold (subo x, x) -> 0 + no borrow
   if (N0 == N1)
     return CombineTo(N, DAG.getConstant(0, DL, VT),
                      DAG.getConstant(0, DL, CarryVT));
 
+  ConstantSDNode *N1C = getAsNonOpaqueConstant(N1);
+
+  // fold (subox, c) -> (addo x, -c)
+  if (IsSigned && N1C && !N1C->getAPIntValue().isMinSignedValue()) {
+    return DAG.getNode(ISD::SADDO, DL, N->getVTList(), N0,
+                       DAG.getConstant(-N1C->getAPIntValue(), DL, VT));
+  }
+
   // fold (subo x, 0) -> x + no borrow
   if (isNullOrNullSplat(N1))
     return CombineTo(N, N0, DAG.getConstant(0, DL, CarryVT));
 
   // Canonicalize (usubo -1, x) -> ~x, i.e. (xor x, -1) + no borrow
   if (!IsSigned && isAllOnesOrAllOnesSplat(N0))
     return CombineTo(N, DAG.getNode(ISD::XOR, DL, VT, N1, N0),
                      DAG.getConstant(0, DL, CarryVT));

llvm/test/CodeGen/SystemZ/int-ssub-07.ll

; CHECK: br %r14
   %obit = extractvalue {i64, i1} %t, 1
   store i64 %val, i64 *%res
   ret i1 %obit
 }
 
 ; Check the next value up, which must be loaded into a register first.
 define zeroext i1 @f5(i64 %dummy, i64 %a, i64 *%res) {
 ; CHECK-LABEL: f5:
-; CHECK: llilf [[REG1:%r[0-9]+]], 2147483649
-; CHECK: sgr %r3, [[REG1]]
-; CHECK-DAG: stg %r3, 0(%r4)
+; CHECK: llihf [[REG1:%r[0-9]+]], 4294967295
+; CHECK: oilf  [[REG1]], 2147483647
+; CHECK: agr [[REG1]], %r3
+; CHECK-DAG: stg %r0, 0(%r4)
 ; CHECK-DAG: ipm [[REG:%r[0-5]]]
 ; CHECK-DAG: afi [[REG]], 1342177280
 ; CHECK-DAG: risbg %r2, [[REG]], 63, 191, 33
 ; CHECK: br %r14
   %t = call {i64, i1} @llvm.ssub.with.overflow.i64(i64 %a, i64 2147483649)
   %val = extractvalue {i64, i1} %t, 0
   %obit = extractvalue {i64, i1} %t, 1
   store i64 %val, i64 *%res
; CHECK: br %r14
   %obit = extractvalue {i64, i1} %t, 1
   store i64 %val, i64 *%res
   ret i1 %obit
 }
 
 ; Check the next value down, which must use register subtraction instead.
 define zeroext i1 @f10(i64 %dummy, i64 %a, i64 *%res) {
 ; CHECK-LABEL: f10:
-; CHECK: lgfi [[REG1:%r[0-9]+]], -2147483648
-; CHECK: sgr %r3, [[REG1]]
-; CHECK-DAG: stg %r3, 0(%r4)
+; CHECK: llilh [[REG1]], 32768
+; CHECK: agr [[REG1]], %r3
+; CHECK-DAG: stg %r0, 0(%r4)
 ; CHECK-DAG: ipm [[REG:%r[0-5]]]
 ; CHECK-DAG: afi [[REG]], 1342177280
 ; CHECK-DAG: risbg %r2, [[REG]], 63, 191, 33
 ; CHECK: br %r14
   %t = call {i64, i1} @llvm.ssub.with.overflow.i64(i64 %a, i64 -2147483648)
   %val = extractvalue {i64, i1} %t, 0
   %obit = extractvalue {i64, i1} %t, 1
   store i64 %val, i64 *%res

llvm/test/CodeGen/X86/sub-with-overflow.ll

 
 declare i32 @printf(i8*, ...) nounwind
 declare {i32, i1} @llvm.ssub.with.overflow.i32(i32, i32)
 declare {i32, i1} @llvm.usub.with.overflow.i32(i32, i32)
 
 define i1 @func3(i32 %x) nounwind {
 ; CHECK-LABEL: func3:
 ; CHECK:       # %bb.0: # %entry
-; CHECK-NEXT:    cmpl $1, {{[0-9]+}}(%esp)
+; CHECK-NEXT:    movl {{[0-9]+}}(%esp), %eax
+; CHECK-NEXT:    decl %eax
 ; CHECK-NEXT:    seto %al
 ; CHECK-NEXT:    retl
 entry:
   %t = call {i32, i1} @llvm.ssub.with.overflow.i32(i32 %x, i32 1)
   %obit = extractvalue {i32, i1} %t, 1
   ret i1 %obit
 
 }

llvm/test/CodeGen/X86/xaluo.ll

 ; NOTE: Assertions have been autogenerated by utils/update_llc_test_checks.py
-; RUN: llc -mtriple=x86_64-darwin-unknown < %s | FileCheck %s --check-prefix=SDAG
+; RUN: llc -mtriple=x86_64-darwin-unknown < %s | FileCheck %s --check-prefix=SDAG --check-prefix=GENERIC
 ; RUN: llc -mtriple=x86_64-darwin-unknown -fast-isel -fast-isel-abort=1 < %s | FileCheck %s --check-prefix=FAST
-; RUN: llc -mtriple=x86_64-darwin-unknown -mcpu=knl < %s | FileCheck %s --check-prefix=SDAG
+; RUN: llc -mtriple=x86_64-darwin-unknown -mcpu=knl < %s | FileCheck %s --check-prefix=SDAG --check-prefix=KNL
 
 ;
 ; Get the actual value of the overflow bit.
 ;
 ; SADDO reg, reg
 define zeroext i1 @saddoi8(i8 signext %v1, i8 signext %v2, i8* %res) {
 ; SDAG-LABEL: saddoi8:
 ; SDAG:       ## %bb.0:
; FAST-NEXT:    retq
   %ret = select i1 %obit, i32 %v1, i32 %v2
   %val = extractvalue {i32, i1} %t, 0
   store i32 %val, i32* %x
   ret i32 %ret
 }
 
 ; Make sure we select a DEC for both the data use and the flag use.
 define i32 @decovfselectstore(i32 %v1, i32 %v2, i32* %x) {
-; SDAG-LABEL: decovfselectstore:
-; SDAG:       ## %bb.0:
-; SDAG-NEXT:    movl %esi, %eax
-; SDAG-NEXT:    movl %edi, %ecx
-; SDAG-NEXT:    decl %ecx
-; SDAG-NEXT:    cmovol %edi, %eax
-; SDAG-NEXT:    movl %ecx, (%rdx)
-; SDAG-NEXT:    retq
+; GENERIC-LABEL: decovfselectstore:
+; GENERIC:       ## %bb.0:
+; GENERIC-NEXT:    movl %esi, %eax
+; GENERIC-NEXT:    movl %edi, %ecx
+; GENERIC-NEXT:    decl %ecx
+; GENERIC-NEXT:    cmovol %edi, %eax
+; GENERIC-NEXT:    movl %ecx, (%rdx)
+; GENERIC-NEXT:    retq
 ;
 ; FAST-LABEL: decovfselectstore:
 ; FAST:       ## %bb.0:
 ; FAST-NEXT:    movl %esi, %eax
 ; FAST-NEXT:    movl %edi, %ecx
 ; FAST-NEXT:    decl %ecx
 ; FAST-NEXT:    cmovol %edi, %eax
 ; FAST-NEXT:    movl %ecx, (%rdx)
 ; FAST-NEXT:    retq
+;
+; KNL-LABEL: decovfselectstore:
+; KNL:       ## %bb.0:
+; KNL-NEXT:    movl %esi, %eax
+; KNL-NEXT:    movl %edi, %ecx
+; KNL-NEXT:    addl $-1, %ecx
+; KNL-NEXT:    cmovol %edi, %eax
+; KNL-NEXT:    movl %ecx, (%rdx)
+; KNL-NEXT:    retq
   %t = call {i32, i1} @llvm.ssub.with.overflow.i32(i32 %v1, i32 1)
   %obit = extractvalue {i32, i1} %t, 1
   %ret = select i1 %obit, i32 %v1, i32 %v2
   %val = extractvalue {i32, i1} %t, 0
   store i32 %val, i32* %x
   ret i32 %ret
 }