[CodeGenPrepare] Bail out of usubo creation if sub's parent is not the same as the comparison

[AZero13]

We match uadd's behavior here.

Codegen comparison: https://godbolt.org/z/x8j4EhGno

[llvmbot]

@llvm/pr-subscribers-backend-x86

Author: AZero13 (AZero13)

Changes

We match uadd's behavior here.

Codegen comparison: https://godbolt.org/z/x8j4EhGno

---

Full diff: https://github.com/llvm/llvm-project/pull/160358.diff

2 Files Affected:

• (modified) llvm/lib/CodeGen/CodeGenPrepare.cpp (+6)
• (modified) llvm/test/CodeGen/X86/usub_inc_iv.ll (+5-5)

diff --git a/llvm/lib/CodeGen/CodeGenPrepare.cpp b/llvm/lib/CodeGen/CodeGenPrepare.cpp
index d290f202f3cca..eb73d01b3558c 100644
--- a/llvm/lib/CodeGen/CodeGenPrepare.cpp
+++ b/llvm/lib/CodeGen/CodeGenPrepare.cpp
@@ -1749,6 +1749,12 @@ bool CodeGenPrepare::combineToUSubWithOverflow(CmpInst *Cmp,
                                  Sub->hasNUsesOrMore(1)))
     return false;
 
+  // We don't want to move around uses of condition values this late, so we
+  // check if it is legal to create the call to the intrinsic in the basic
+  // block containing the icmp.
+  if (Sub->getParent() != Cmp->getParent() && !Sub->hasOneUse())
+    return false;
+
   if (!replaceMathCmpWithIntrinsic(Sub, Sub->getOperand(0), Sub->getOperand(1),
                                    Cmp, Intrinsic::usub_with_overflow))
     return false;
diff --git a/llvm/test/CodeGen/X86/usub_inc_iv.ll b/llvm/test/CodeGen/X86/usub_inc_iv.ll
index 88bfddb51f2d4..ff06aaabd1b0c 100644
--- a/llvm/test/CodeGen/X86/usub_inc_iv.ll
+++ b/llvm/test/CodeGen/X86/usub_inc_iv.ll
@@ -303,14 +303,14 @@ define i32 @test_06(ptr %p, i64 %len, i32 %x) {
 ; CHECK-NEXT:    br label [[LOOP:%.*]]
 ; CHECK:       loop:
 ; CHECK-NEXT:    [[IV:%.*]] = phi i64 [ [[MATH:%.*]], [[BACKEDGE:%.*]] ], [ [[LEN:%.*]], [[ENTRY:%.*]] ]
-; CHECK-NEXT:    [[TMP0:%.*]] = call { i64, i1 } @llvm.usub.with.overflow.i64(i64 [[IV]], i64 1)
-; CHECK-NEXT:    [[MATH]] = extractvalue { i64, i1 } [[TMP0]], 0
-; CHECK-NEXT:    [[OV:%.*]] = extractvalue { i64, i1 } [[TMP0]], 1
+; CHECK-NEXT:    [[OV:%.*]] = icmp eq i64 [[IV]], 0
 ; CHECK-NEXT:    br i1 [[OV]], label [[EXIT:%.*]], label [[BACKEDGE]]
 ; CHECK:       backedge:
-; CHECK-NEXT:    [[SUNKADDR:%.*]] = mul i64 [[MATH]], 4
+; CHECK-NEXT:    [[SUNKADDR:%.*]] = mul i64 [[IV]], 4
 ; CHECK-NEXT:    [[SUNKADDR1:%.*]] = getelementptr i8, ptr [[P:%.*]], i64 [[SUNKADDR]]
-; CHECK-NEXT:    [[LOADED:%.*]] = load atomic i32, ptr [[SUNKADDR1]] unordered, align 4
+; CHECK-NEXT:    [[SUNKADDR2:%.*]] = getelementptr i8, ptr [[SUNKADDR1]], i64 -4
+; CHECK-NEXT:    [[LOADED:%.*]] = load atomic i32, ptr [[SUNKADDR2]] unordered, align 4
+; CHECK-NEXT:    [[MATH]] = add i64 [[IV]], -1
 ; CHECK-NEXT:    [[COND_2:%.*]] = icmp eq i32 [[LOADED]], [[X:%.*]]
 ; CHECK-NEXT:    br i1 [[COND_2]], label [[FAILURE:%.*]], label [[LOOP]]
 ; CHECK:       exit:

[AZero13]

@arsenm Thoughts on this?

[arsenm]

I'm surprised we have wholly separate implementations for these cases. The tail of the function is identical, except for the opcode. Can you refactor this so the two share the code

[AZero13]

I'm surprised we have wholly separate implementations for these cases. The tail of the function is identical, except for the opcode. Can you refactor this so the two share the code

See #160327

[AZero13]

@arsenm I don't want to put the cart before the horse. I have another PR dedicated to that with its own new matcher.

[AZero13]

Thank you. @arsenm Could you please merge?