Skip to content

Conversation

@sergachev
Copy link
Contributor

Remove smax from smax(smin(abs(x), y), 0) if y >= 0.

@llvmbot
Copy link
Member

llvmbot commented Oct 3, 2024

@llvm/pr-subscribers-llvm-transforms

Author: Ilia Sergachev (sergachev)

Changes

Remove smax from smax(smin(abs(x), y), 0) if y >= 0.


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

2 Files Affected:

  • (modified) llvm/lib/Transforms/InstCombine/InstCombineCalls.cpp (+7)
  • (modified) llvm/test/Transforms/InstCombine/abs-intrinsic.ll (+22)
diff --git a/llvm/lib/Transforms/InstCombine/InstCombineCalls.cpp b/llvm/lib/Transforms/InstCombine/InstCombineCalls.cpp
index f7a9406791801c..59b92bc833b673 100644
--- a/llvm/lib/Transforms/InstCombine/InstCombineCalls.cpp
+++ b/llvm/lib/Transforms/InstCombine/InstCombineCalls.cpp
@@ -1770,6 +1770,13 @@ Instruction *InstCombinerImpl::visitCallInst(CallInst &CI) {
       }
     }
 
+    // smax(smin(abs(x), y), 0) -> smin(abs(x), y) where y >= 0
+    if (match(I0, m_SMin(m_c_Intrinsic<Intrinsic::abs>(m_Value(), m_Value()),
+                         m_NonNegative())) &&
+        match(I1, m_Zero())) {
+      return replaceInstUsesWith(*II, I0);
+    }
+
     // umin(i1 X, i1 Y) -> and i1 X, Y
     // smax(i1 X, i1 Y) -> and i1 X, Y
     if ((IID == Intrinsic::umin || IID == Intrinsic::smax) &&
diff --git a/llvm/test/Transforms/InstCombine/abs-intrinsic.ll b/llvm/test/Transforms/InstCombine/abs-intrinsic.ll
index f7c639f1d8e6ba..53ad2057373569 100644
--- a/llvm/test/Transforms/InstCombine/abs-intrinsic.ll
+++ b/llvm/test/Transforms/InstCombine/abs-intrinsic.ll
@@ -847,3 +847,25 @@ cond.end:
   %r = phi i32 [ %0, %cond.true ], [ 0, %entry ]
   ret i32 %r
 }
+
+define i32 @test_abs_min_max_nonneg(i32 %0) {
+; CHECK-LABEL: @test_abs_min_max_nonneg(
+; CHECK-NEXT: call i32 @llvm.abs.i32
+; CHECK-NEXT: call i32 @llvm.smin.i32
+; CHECK-NOT: call i32 @llvm.smax.i32
+  %2 = call i32 @llvm.abs.i32(i32 %0, i1 false)
+  %3 = call i32 @llvm.smin.i32(i32 %2, i32 5)
+  %4 = call i32 @llvm.smax.i32(i32 %3, i32 0)
+  ret i32 %4
+}
+
+define i32 @test_abs_min_max_unknown(i32 %0, i32 %1) {
+; CHECK-LABEL: @test_abs_min_max_unknown(
+; CHECK-NEXT: call i32 @llvm.abs.i32
+; CHECK-NEXT: call i32 @llvm.smin.i32
+; CHECK-NEXT: call i32 @llvm.smax.i32
+  %3 = call i32 @llvm.abs.i32(i32 %0, i1 false)
+  %4 = call i32 @llvm.smin.i32(i32 %3, i32 %1)
+  %5 = call i32 @llvm.smax.i32(i32 %4, i32 0)
+  ret i32 %5
+}

Copy link
Member

@dtcxzyw dtcxzyw left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is incorrect when x==INT_MIN: https://alive2.llvm.org/ce/z/X4TZPY

BTW, isn't it already handled by CVP/SCCP?

@goldsteinn
Copy link
Contributor

It is incorrect when x==INT_MIN: https://alive2.llvm.org/ce/z/X4TZPY

It is valid if you make INT_MIN ub.

Remove smax from smax(smin(abs(x), y), 0) if y >= 0
@sergachev
Copy link
Contributor Author

BTW, isn't it already handled by CVP/SCCP?

It isn't handled at the moment: https://alive2.llvm.org/ce/z/5o8Y-m


// smax(smin(abs(x), y), 0) -> smin(abs(x), y) where y >= 0
if (IID == Intrinsic::smax &&
match(I0, m_SMin(m_c_Intrinsic<Intrinsic::abs>(m_Value(), m_Value()),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Still need to be m_c_Intrinsic<..::abs>(m_Value(), m_One()) (And your tests will need toupdated correspondingly.)

@nikic
Copy link
Contributor

nikic commented Oct 3, 2024

BTW, isn't it already handled by CVP/SCCP?

It isn't handled at the moment: https://alive2.llvm.org/ce/z/5o8Y-m

It's not handled because the transform is incorrect. The variant with int_min_is_poison set is already handled: https://alive2.llvm.org/ce/z/Ah_8wT

@sergachev
Copy link
Contributor Author

Thank you for the clarifications!

@sergachev sergachev closed this Oct 4, 2024
@sergachev sergachev deleted the users/sergachev/instcombine_max_abs branch October 4, 2024 09:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants