Skip to content

Conversation

bobby-b-song
Copy link

Addresses the #141479 issue. By adding the following optimiztion

define i1 @src(i32 %0, i32 %1) local_unnamed_addr #0 {
common.ret:
  %2 = xor i32 %0, -1
  %3 = icmp ule i32 %1, %2
  %4 = xor i32 %1, -1
  %5 = icmp ugt i32 %0, %4
  %common.ret.op = and i1 %3, %5
  ret i1 %common.ret.op
}

to

define noundef i1 @tgt(i32 %0, i32 %1) local_unnamed_addr #0 {
common.ret:
  ret i1 false
}

Copy link

Thank you for submitting a Pull Request (PR) to the LLVM Project!

This PR will be automatically labeled and the relevant teams will be notified.

If you wish to, you can add reviewers by using the "Reviewers" section on this page.

If this is not working for you, it is probably because you do not have write permissions for the repository. In which case you can instead tag reviewers by name in a comment by using @ followed by their GitHub username.

If you have received no comments on your PR for a week, you can request a review by "ping"ing the PR by adding a comment “Ping”. The common courtesy "ping" rate is once a week. Please remember that you are asking for valuable time from other developers.

If you have further questions, they may be answered by the LLVM GitHub User Guide.

You can also ask questions in a comment on this PR, on the LLVM Discord or on the forums.

@llvmbot llvmbot added llvm:instcombine Covers the InstCombine, InstSimplify and AggressiveInstCombine passes llvm:transforms labels May 29, 2025
@bobby-b-song bobby-b-song marked this pull request as draft May 29, 2025 15:16
@llvmbot
Copy link
Member

llvmbot commented May 29, 2025

@llvm/pr-subscribers-llvm-analysis

@llvm/pr-subscribers-llvm-transforms

Author: Bobby SONG (bobby-b-song)

Changes

Addresses the #141479 issue. By adding the following optimiztion

define i1 @<!-- -->src(i32 %0, i32 %1) local_unnamed_addr #<!-- -->0 {
common.ret:
  %2 = xor i32 %0, -1
  %3 = icmp ule i32 %1, %2
  %4 = xor i32 %1, -1
  %5 = icmp ugt i32 %0, %4
  %common.ret.op = and i1 %3, %5
  ret i1 %common.ret.op
}

to

define noundef i1 @<!-- -->tgt(i32 %0, i32 %1) local_unnamed_addr #<!-- -->0 {
common.ret:
  ret i1 false
}

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

1 Files Affected:

  • (added) llvm/test/Transforms/InstCombine/and-comparison-not-always-false.ll (+21)
diff --git a/llvm/test/Transforms/InstCombine/and-comparison-not-always-false.ll b/llvm/test/Transforms/InstCombine/and-comparison-not-always-false.ll
new file mode 100644
index 0000000000000..174d97d30bcf8
--- /dev/null
+++ b/llvm/test/Transforms/InstCombine/and-comparison-not-always-false.ll
@@ -0,0 +1,21 @@
+; NOTE: Assertions have been autogenerated by utils/update_test_checks.py UTC_ARGS: --version 5
+; RUN: opt < %s -passes=instcombine -S | FileCheck %s
+define i1 @test(i32 %0, i32 %1) {
+; CHECK-LABEL: define i1 @test(
+; CHECK-SAME: i32 [[TMP0:%.*]], i32 [[TMP1:%.*]]) {
+; CHECK-NEXT:  [[COMMON_RET:.*:]]
+; CHECK-NEXT:    [[TMP2:%.*]] = xor i32 [[TMP0]], -1
+; CHECK-NEXT:    [[TMP3:%.*]] = icmp ule i32 [[TMP1]], [[TMP2]]
+; CHECK-NEXT:    [[TMP4:%.*]] = xor i32 [[TMP1]], -1
+; CHECK-NEXT:    [[TMP5:%.*]] = icmp ugt i32 [[TMP0]], [[TMP4]]
+; CHECK-NEXT:    [[COMMON_RET_OP:%.*]] = and i1 [[TMP3]], [[TMP5]]
+; CHECK-NEXT:    ret i1 [[COMMON_RET_OP]]
+;
+common.ret:
+  %2 = xor i32 %0, -1
+  %3 = icmp ule i32 %1, %2
+  %4 = xor i32 %1, -1
+  %5 = icmp ugt i32 %0, %4
+  %common.ret.op = and i1 %3, %5
+  ret i1 %common.ret.op
+}

@bobby-b-song bobby-b-song force-pushed the main branch 2 times, most recently from ff8469a to ade7091 Compare May 30, 2025 10:27
@bobby-b-song
Copy link
Author

I've further extended the PR with signed numbers and more inclusive/exclusive ranges, the proof of alive2 is at https://alive2.llvm.org/ce/z/z2VcMn.

Add test for optimization

Extend the case further to signed intergers and more in/exclusive ranges

Update test for optimization
@bobby-b-song bobby-b-song marked this pull request as ready for review May 31, 2025 06:42
@bobby-b-song bobby-b-song requested a review from nikic as a code owner May 31, 2025 06:42
@llvmbot llvmbot added the llvm:analysis Includes value tracking, cost tables and constant folding label May 31, 2025
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.

Miscompilation reproducer: https://alive2.llvm.org/ce/z/yaCs4R

define i1 @src(i32 %0, i32 %1) {
common.ret:
  %2 = xor i32 %0, -1
  %3 = icmp sle i32 %1, %2
  %4 = xor i32 %1, -1
  %5 = icmp ugt i32 %0, %4
  %common.ret.op = and i1 %3, %5
  ret i1 %common.ret.op
}

define i1 @tgt(i32 %0, i32 %1) {
common.ret:
  ret i1 false
}

@dtcxzyw
Copy link
Member

dtcxzyw commented Aug 15, 2025

As I commented in #141479 (comment), both LHS and RHS are uadd-overflow check idiom. I am not sure if it is profitable to canonicalize them into extractvalue uadd.overflow(x, y), 1. If not, we can add a helper function to recognize such overflow-checking patterns. cc @nikic @AZero13

// (X <= ~Y) && (Y > ~X) --> 0
CmpPredicate Pred0, Pred1;
if (match(Op0,
m_c_ICmp(Pred0, m_Value(X), m_c_Xor(m_Value(Y), m_AllOnes()))) &&
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
m_c_ICmp(Pred0, m_Value(X), m_c_Xor(m_Value(Y), m_AllOnes()))) &&
m_c_ICmp(Pred0, m_Value(X), m_Not(m_Value(Y)))) &&

if (match(Op0,
m_c_ICmp(Pred0, m_Value(X), m_c_Xor(m_Value(Y), m_AllOnes()))) &&
match(Op1, m_c_ICmp(Pred1, m_Specific(Y),
m_c_Xor(m_Specific(X), m_AllOnes())))) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
m_c_Xor(m_Specific(X), m_AllOnes())))) {
m_Not(m_Specific(X))))) {

@nikic
Copy link
Contributor

nikic commented Aug 31, 2025

As I commented in #141479 (comment), both LHS and RHS are uadd-overflow check idiom. I am not sure if it is profitable to canonicalize them into extractvalue uadd.overflow(x, y), 1. If not, we can add a helper function to recognize such overflow-checking patterns.

We have m_UAddWithOverflow() for this. It does match the ~x < y pattern as well. But it doesn't match the negated overflow check.

@nikic nikic changed the title [InstCombine] Add Missed Optimization [InstCombine] Optimize and of overflow checks Aug 31, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
llvm:analysis Includes value tracking, cost tables and constant folding llvm:instcombine Covers the InstCombine, InstSimplify and AggressiveInstCombine passes llvm:transforms
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants