Skip to content

Conversation

@dtcxzyw
Copy link
Member

@dtcxzyw dtcxzyw commented Feb 5, 2025

Alive2: https://alive2.llvm.org/ce/z/6zLAYp

Note: We can also apply this fix to the logic below (if (Mask & AMask_NotAllOnes)), but it seems unreachable.

@dtcxzyw dtcxzyw requested a review from nikic as a code owner February 5, 2025 10:12
@llvmbot llvmbot added llvm:instcombine Covers the InstCombine, InstSimplify and AggressiveInstCombine passes llvm:transforms labels Feb 5, 2025
@llvmbot
Copy link
Member

llvmbot commented Feb 5, 2025

@llvm/pr-subscribers-llvm-transforms

Author: Yingwei Zheng (dtcxzyw)

Changes

Alive2: https://alive2.llvm.org/ce/z/6zLAYp

Note: We can also apply this fix to the logic below (if (Mask & AMask_NotAllOnes)), but it seems unreachable.


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

2 Files Affected:

  • (modified) llvm/lib/Transforms/InstCombine/InstCombineAndOrXor.cpp (+4-1)
  • (modified) llvm/test/Transforms/InstCombine/sign-test-and-or.ll (+23)
diff --git a/llvm/lib/Transforms/InstCombine/InstCombineAndOrXor.cpp b/llvm/lib/Transforms/InstCombine/InstCombineAndOrXor.cpp
index 8701f7c28a39fc..29f67f34a017ca 100644
--- a/llvm/lib/Transforms/InstCombine/InstCombineAndOrXor.cpp
+++ b/llvm/lib/Transforms/InstCombine/InstCombineAndOrXor.cpp
@@ -610,8 +610,11 @@ static Value *foldLogOpOfMaskedICmps(Value *LHS, Value *RHS, bool IsAnd,
       APInt NewMask = *ConstB & *ConstD;
       if (NewMask == *ConstB)
         return LHS;
-      if (NewMask == *ConstD)
+      if (NewMask == *ConstD) {
+        if (IsLogical)
+          cast<ICmpInst>(RHS)->setSameSign(false);
         return RHS;
+      }
     }
 
     if (Mask & AMask_NotAllOnes) {
diff --git a/llvm/test/Transforms/InstCombine/sign-test-and-or.ll b/llvm/test/Transforms/InstCombine/sign-test-and-or.ll
index 65363620563bed..3e9ff63869d646 100644
--- a/llvm/test/Transforms/InstCombine/sign-test-and-or.ll
+++ b/llvm/test/Transforms/InstCombine/sign-test-and-or.ll
@@ -349,6 +349,29 @@ define i1 @test9_logical(i32 %a) {
   ret i1 %or.cond
 }
 
+define i1 @test9_logical_samesign(i32 %a) {
+; CHECK-LABEL: @test9_logical_samesign(
+; CHECK-NEXT:    [[CMP2:%.*]] = icmp sgt i32 [[A:%.*]], -1
+; CHECK-NEXT:    ret i1 [[CMP2]]
+;
+  %masked = and i32 %a, -1073741825
+  %cmp1 = icmp eq i32 %masked, 0
+  %cmp2 = icmp samesign sgt i32 %a, -1
+  %or.cond = select i1 %cmp1, i1 true, i1 %cmp2
+  ret i1 %or.cond
+}
+
+define i1 @test_logical_or_icmp_icmp_samesign(i32 %a) {
+; CHECK-LABEL: @test_logical_or_icmp_icmp_samesign(
+; CHECK-NEXT:    [[CMP2:%.*]] = icmp sgt i32 [[A:%.*]], -1
+; CHECK-NEXT:    ret i1 [[CMP2]]
+;
+  %cmp1 = icmp eq i32 %a, 0
+  %cmp2 = icmp samesign sgt i32 %a, -1
+  %or = select i1 %cmp1, i1 true, i1 %cmp2
+  ret i1 %or
+}
+
 define i1 @test10(i32 %a) {
 ; CHECK-LABEL: @test10(
 ; CHECK-NEXT:    [[OR_COND:%.*]] = icmp ult i32 [[A:%.*]], 2

Copy link
Contributor

@andjo403 andjo403 left a comment

Choose a reason for hiding this comment

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

Looks good to me but wait for other reviewers

Copy link
Contributor

@goldsteinn goldsteinn left a comment

Choose a reason for hiding this comment

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

LGTM

@dtcxzyw dtcxzyw merged commit 9cd83d6 into llvm:main Feb 7, 2025
8 checks passed
@dtcxzyw dtcxzyw deleted the drop-samesign branch February 7, 2025 03:56
Icohedron pushed a commit to Icohedron/llvm-project that referenced this pull request Feb 11, 2025
Alive2: https://alive2.llvm.org/ce/z/6zLAYp

Note: We can also apply this fix to the logic below (`if (Mask &
AMask_NotAllOnes)`), but it seems unreachable.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

llvm:instcombine Covers the InstCombine, InstSimplify and AggressiveInstCombine passes llvm:transforms

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants