[InstCombine] Fold usub_sat((sub nuw C1, A), C2) to usub_sat(C1 - C2, A) or 0 #82280

elhewaty · 2024-02-19T21:43:08Z

[InstCombine] Add test coverage for folding usub_sat((sub nuw C1, A), C2) to usub_sat(C1 - C2, A) or 0(NFC)
[InstCombine] Fold usub_sat((sub nuw C1, A), C2) to usub_sat(C1 - C2, A) or 0
Fixes: [InstCombine] Missed optimization: Fold usub_sat((sub nuw C1, A), C2) to usub_sat(C1 - C2, A) or 0 #82177
Alive2: https://alive2.llvm.org/ce/z/Q7mMC3

llvmbot · 2024-02-19T21:43:36Z

@llvm/pr-subscribers-llvm-transforms

Author: None (elhewaty)

Changes

[InstCombine] Add test coverage for folding usub_sat((sub nuw C1, A), C2) to usub_sat(C1 - C2, A) or 0(NFC)
[InstCombine] Fold usub_sat((sub nuw C1, A), C2) to usub_sat(C1 - C2, A) or 0
Fixes: [InstCombine] Missed optimization: Fold usub_sat((sub nuw C1, A), C2) to usub_sat(C1 - C2, A) or 0 #82177
Alive2: https://alive2.llvm.org/ce/z/4w-77P

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

2 Files Affected:

(modified) llvm/lib/Transforms/InstCombine/InstCombineCalls.cpp (+17)
(modified) llvm/test/Transforms/InstCombine/unsigned_saturated_sub.ll (+71)

diff --git a/llvm/lib/Transforms/InstCombine/InstCombineCalls.cpp b/llvm/lib/Transforms/InstCombine/InstCombineCalls.cpp
index 5266808c5abab4..7f59ad1d61d91e 100644
--- a/llvm/lib/Transforms/InstCombine/InstCombineCalls.cpp
+++ b/llvm/lib/Transforms/InstCombine/InstCombineCalls.cpp
@@ -2139,6 +2139,23 @@ Instruction *InstCombinerImpl::visitCallInst(CallInst &CI) {
       }
     }
 
+    // usub_sat((sub nuw C1, A), C2) -> usub_sat(C1 - C2, A) if C2 u< C1
+    // usub_sat((sub nuw C1, A), C2) -> 0 otherwise
+    const APInt *C1, *C2;
+    Value *A;
+    if (IID == Intrinsic::usub_sat &&
+        match(Arg0, m_OneUse(m_NUWSub(m_APInt(C1), m_Value(A)))) &&
+        match(Arg1, m_APInt(C2)) && SI->hasOneUse()) {
+
+      if (C2->ult(*C1)) {
+        auto *New = Builder.CreateBinaryIntrinsic(
+            Intrinsic::usub_sat, ConstantInt::get(A->getType(), *C1 - *C2), A);
+        return replaceInstUsesWith(*SI, New);
+      } else {
+        return replaceInstUsesWith(*SI, ConstantInt::get(Ty, 0));
+      }
+    }
+
     // ssub.sat(X, C) -> sadd.sat(X, -C) if C != MIN
     Constant *C;
     if (IID == Intrinsic::ssub_sat && match(Arg1, m_Constant(C)) &&
diff --git a/llvm/test/Transforms/InstCombine/unsigned_saturated_sub.ll b/llvm/test/Transforms/InstCombine/unsigned_saturated_sub.ll
index 5cece931b8d987..4d60a7cc5a54bf 100644
--- a/llvm/test/Transforms/InstCombine/unsigned_saturated_sub.ll
+++ b/llvm/test/Transforms/InstCombine/unsigned_saturated_sub.ll
@@ -7,6 +7,77 @@
 declare void @use(i64)
 declare void @usei32(i32)
 declare void @usei1(i1)
+declare i32 @llvm.usub.sat.i32(i32, i32)
+declare i16 @llvm.usub.sat.i16(i16, i16)
+
+; usub_sat((sub nuw C1, A), C2) to usub_sat(C1 - C2, A) or 0
+
+define i32 @usub_sat_C1_C2(i32 %a){
+; CHECK-LABEL: @usub_sat_C1_C2(
+; CHECK-NEXT:    [[COND:%.*]] = call i32 @llvm.usub.sat.i32(i32 50, i32 [[A:%.*]])
+; CHECK-NEXT:    ret i32 [[COND]]
+;
+  %add = sub nuw i32 64, %a
+  %cond = call i32 @llvm.usub.sat.i32(i32 %add, i32 14)
+  ret i32 %cond
+}
+
+define i32 @usub_sat_C1_C2_produce_0(i32 %a){
+; CHECK-LABEL: @usub_sat_C1_C2_produce_0(
+; CHECK-NEXT:    ret i32 0
+;
+  %add = sub nuw i32 14, %a
+  %cond = call i32 @llvm.usub.sat.i32(i32 %add, i32 14)
+  ret i32 %cond
+}
+
+define i32 @usub_sat_C1_C2_produce_0_too(i32 %a){
+; CHECK-LABEL: @usub_sat_C1_C2_produce_0_too(
+; CHECK-NEXT:    ret i32 0
+;
+  %add = sub nuw i32 12, %a
+  %cond = call i32 @llvm.usub.sat.i32(i32 %add, i32 14)
+  ret i32 %cond
+}
+
+; negative tests this souldn't work
+
+define i32 @usub_sat_C1_C2_without_nuw(i32 %a){
+; CHECK-LABEL: @usub_sat_C1_C2_without_nuw(
+; CHECK-NEXT:    [[ADD:%.*]] = sub i32 12, [[A:%.*]]
+; CHECK-NEXT:    [[COND:%.*]] = call i32 @llvm.usub.sat.i32(i32 [[ADD]], i32 14)
+; CHECK-NEXT:    ret i32 [[COND]]
+;
+  %add = sub i32 12, %a
+  %cond = call i32 @llvm.usub.sat.i32(i32 %add, i32 14)
+  ret i32 %cond
+}
+
+define i32 @usub_sat_C1_C2_more_than_one_use_with_add(i32 %a){
+; CHECK-LABEL: @usub_sat_C1_C2_more_than_one_use_with_add(
+; CHECK-NEXT:    [[ADD:%.*]] = sub nuw i32 12, [[A:%.*]]
+; CHECK-NEXT:    [[COND:%.*]] = call i32 @llvm.usub.sat.i32(i32 [[ADD]], i32 14)
+; CHECK-NEXT:    call void @usei32(i32 [[ADD]])
+; CHECK-NEXT:    ret i32 [[COND]]
+;
+  %add = sub nuw i32 12, %a
+  %cond = call i32 @llvm.usub.sat.i32(i32 %add, i32 14)
+  call void @usei32(i32 %add)
+  ret i32 %cond
+}
+
+define i32 @usub_sat_C1_C2_more_than_one_use_with_cond(i32 %a){
+; CHECK-LABEL: @usub_sat_C1_C2_more_than_one_use_with_cond(
+; CHECK-NEXT:    [[ADD:%.*]] = sub nuw i32 12, [[A:%.*]]
+; CHECK-NEXT:    [[COND:%.*]] = call i32 @llvm.usub.sat.i32(i32 [[ADD]], i32 14)
+; CHECK-NEXT:    call void @usei32(i32 [[COND]])
+; CHECK-NEXT:    ret i32 [[COND]]
+;
+  %add = sub nuw i32 12, %a
+  %cond = call i32 @llvm.usub.sat.i32(i32 %add, i32 14)
+  call void @usei32(i32 %cond)
+  ret i32 %cond
+}
 
 ; (a > b) ? a - b : 0 -> usub.sat(a, b)

elhewaty · 2024-02-19T21:44:36Z

@nikic, @dtcxzyw, @goldsteinn

llvm/lib/Transforms/InstCombine/InstCombineCalls.cpp

llvm/test/Transforms/InstCombine/unsigned_saturated_sub.ll

nikic · 2024-03-07T21:42:55Z

Reverse ping

elhewaty · 2024-03-07T21:56:16Z

@nikic, @XChy Sorry I will update it now

* vector test

Do we need the vector test?

* test with commuted sub like (sub nuw i32 %a, 12), proof: https://alive2.llvm.org/ce/z/kVXDgW

For this test I think the optimizer converts sub to add and then add a negative number, Is there is a way to match additions with over flow like this:

%res = call {i8, i1} @llvm.uadd.with.overflow.i8(i8 %c1, i8 %c2)
  %overflow = extractvalue {i8, i1} %res, 1
  %notoverflow = xor i1 %overflow, -1
  call void @llvm.assume(i1 %notoverflow)

  %add = sub nuw i8 %a, %c1
  %cond = call i8 @llvm.usub.sat.i8(i8 %add, i8 %c2)
  ret i8 %cond

XChy · 2024-03-09T10:08:20Z

Do we need the vector test?

We always add vector tests if the pattern could handle vectors.

For this test I think the optimizer converts sub to add and then add a negative number, Is there is a way to match additions with over flow like this:

I think APInt::uadd_ov may help. Or if the commuted pattern is difficult now, I think it's OK solve the pattern in original issue first, and then propose another patch for the commuted one, since they are independent.

nikic · 2024-03-09T10:24:04Z

Yes, we should not try to handle the commuted pattern in this patch.

PR Link: llvm/llvm-project#82280

dtcxzyw

LGTM. Thank you!

nikic · 2024-03-10T15:04:50Z

llvm/lib/Transforms/InstCombine/InstCombineCalls.cpp

+    const APInt *C1, *C2;
+    Value *A;
+    if (IID == Intrinsic::usub_sat &&
+        match(Arg0, m_OneUse(m_NUWSub(m_APInt(C1), m_Value(A)))) &&


I think we can drop this m_OneUse as well. We'll get the same number of instructions, but smaller dependence distance.

nikic · 2024-03-10T15:07:31Z

llvm/lib/Transforms/InstCombine/InstCombineCalls.cpp

+      } else {
+        return replaceInstUsesWith(*SI, ConstantInt::get(Ty, 0));
+      }
+    }


An alternative way to phrase this would be like this: https://alive2.llvm.org/ce/z/_Gw9fQ

This way we don't have to check the C2->ult(C1) condition explicitly, which means that we can more easily generalize this to non-splat vectors.

It would look something like this:

if (IID == Intrinsic::usub_sat && match(Arg0, m_NUWSub(m_ImmConstant(C1), m_Value(A))) && match(Arg1, m_ImmConstant(C2))) { auto *NewC = Builder.CreateBinaryIntrinsic(Intrinsic::usub_sat, C1, C2); auto *NewSub = Builder.CreateBinaryIntrinsic(Intrinsic::usub_sat, NewC, A); return replaceInstUsesWith(*SI, NewSub); }

What do you think?

Worth noting that when phrased like this, the following generalization to uadd.sat + add nuw also holds: https://alive2.llvm.org/ce/z/Y4VGfs

XChy

LGTM

nikic

LGTM with some nits.

llvm/lib/Transforms/InstCombine/InstCombineCalls.cpp

llvm/test/Transforms/InstCombine/unsigned_saturated_sub.ll

… C2) to usub_sat(C1 - C2, A) or 0(NFC)

… A) or 0

elhewaty requested a review from nikic as a code owner February 19, 2024 21:43

llvmbot added the llvm:transforms label Feb 19, 2024

XChy requested review from dtcxzyw and goldsteinn February 20, 2024 05:53

XChy reviewed Feb 20, 2024

View reviewed changes

dtcxzyw reviewed Feb 20, 2024

View reviewed changes

llvm/test/Transforms/InstCombine/unsigned_saturated_sub.ll Outdated Show resolved Hide resolved

elhewaty force-pushed the usub_sat branch from 7668aa9 to 4e21d4e Compare March 9, 2024 19:29

elhewaty requested review from dtcxzyw and XChy March 9, 2024 19:29

dtcxzyw added a commit to dtcxzyw/llvm-opt-benchmark that referenced this pull request Mar 10, 2024

pre-commit: test PR82280

dbe93a9

PR Link: llvm/llvm-project#82280

dtcxzyw mentioned this pull request Mar 10, 2024

pre-commit: test PR82280 dtcxzyw/llvm-opt-benchmark#365

Closed

dtcxzyw approved these changes Mar 10, 2024

View reviewed changes

nikic reviewed Mar 10, 2024

View reviewed changes

elhewaty force-pushed the usub_sat branch from 4e21d4e to 1014fdc Compare March 10, 2024 22:28

elhewaty requested a review from nikic March 10, 2024 22:31

XChy approved these changes Mar 11, 2024

View reviewed changes

nikic approved these changes Mar 11, 2024

View reviewed changes

llvm/lib/Transforms/InstCombine/InstCombineCalls.cpp Outdated Show resolved Hide resolved

llvm/test/Transforms/InstCombine/unsigned_saturated_sub.ll Show resolved Hide resolved

elhewaty added 2 commits March 11, 2024 14:08

[InstCombine] Add test coverage for folding usub_sat((sub nuw C1, A),…

e03339a

… C2) to usub_sat(C1 - C2, A) or 0(NFC)

[InstCombine] Fold usub_sat((sub nuw C1, A), C2) to usub_sat(C1 - C2,…

84560f7

… A) or 0

elhewaty force-pushed the usub_sat branch from 1014fdc to 84560f7 Compare March 11, 2024 12:29

elhewaty requested a review from nikic March 11, 2024 12:29

nikic merged commit 3f302ea into llvm:main Mar 11, 2024
4 checks passed

elhewaty deleted the usub_sat branch March 11, 2024 14:29

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[InstCombine] Fold usub_sat((sub nuw C1, A), C2) to usub_sat(C1 - C2, A) or 0 #82280

[InstCombine] Fold usub_sat((sub nuw C1, A), C2) to usub_sat(C1 - C2, A) or 0 #82280

elhewaty commented Feb 19, 2024 •

edited

llvmbot commented Feb 19, 2024

elhewaty commented Feb 19, 2024 •

edited

nikic commented Mar 7, 2024

elhewaty commented Mar 7, 2024 •

edited

XChy commented Mar 9, 2024 •

edited

nikic commented Mar 9, 2024

dtcxzyw left a comment

nikic Mar 10, 2024

nikic Mar 10, 2024

nikic Mar 10, 2024

XChy left a comment

nikic left a comment

[InstCombine] Fold usub_sat((sub nuw C1, A), C2) to usub_sat(C1 - C2, A) or 0 #82280

[InstCombine] Fold usub_sat((sub nuw C1, A), C2) to usub_sat(C1 - C2, A) or 0 #82280

Conversation

elhewaty commented Feb 19, 2024 • edited

llvmbot commented Feb 19, 2024

elhewaty commented Feb 19, 2024 • edited

nikic commented Mar 7, 2024

elhewaty commented Mar 7, 2024 • edited

XChy commented Mar 9, 2024 • edited

nikic commented Mar 9, 2024

dtcxzyw left a comment

Choose a reason for hiding this comment

nikic Mar 10, 2024

Choose a reason for hiding this comment

nikic Mar 10, 2024

Choose a reason for hiding this comment

nikic Mar 10, 2024

Choose a reason for hiding this comment

XChy left a comment

Choose a reason for hiding this comment

nikic left a comment

Choose a reason for hiding this comment

elhewaty commented Feb 19, 2024 •

edited

elhewaty commented Feb 19, 2024 •

edited

elhewaty commented Mar 7, 2024 •

edited

XChy commented Mar 9, 2024 •

edited