Skip to content

Conversation

andjo403
Copy link
Contributor

@andjo403 andjo403 requested review from nikic and dtcxzyw February 26, 2025 11:13
@llvmbot llvmbot added llvm:instcombine Covers the InstCombine, InstSimplify and AggressiveInstCombine passes llvm:transforms labels Feb 26, 2025
@llvmbot
Copy link
Member

llvmbot commented Feb 26, 2025

@llvm/pr-subscribers-llvm-transforms

Author: Andreas Jonson (andjo403)

Changes

proof: https://alive2.llvm.org/ce/z/pu8WmX

fixes #128778


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

3 Files Affected:

  • (modified) llvm/lib/Transforms/InstCombine/InstCombineAndOrXor.cpp (+7-6)
  • (modified) llvm/test/Transforms/InstCombine/eq-of-parts.ll (+2-4)
  • (modified) llvm/test/Transforms/InstCombine/onehot_merge.ll (+6-9)
diff --git a/llvm/lib/Transforms/InstCombine/InstCombineAndOrXor.cpp b/llvm/lib/Transforms/InstCombine/InstCombineAndOrXor.cpp
index 4616ea6ab5487..3649626d18e14 100644
--- a/llvm/lib/Transforms/InstCombine/InstCombineAndOrXor.cpp
+++ b/llvm/lib/Transforms/InstCombine/InstCombineAndOrXor.cpp
@@ -3336,12 +3336,6 @@ Value *InstCombinerImpl::foldAndOrOfICmps(ICmpInst *LHS, ICmpInst *RHS,
     }
   }
 
-  // handle (roughly):
-  // (icmp ne (A & B), C) | (icmp ne (A & D), E)
-  // (icmp eq (A & B), C) & (icmp eq (A & D), E)
-  if (Value *V = foldLogOpOfMaskedICmps(LHS, RHS, IsAnd, IsLogical, Builder, Q))
-    return V;
-
   if (Value *V =
           foldAndOrOfICmpEqConstantAndICmp(LHS, RHS, IsAnd, IsLogical, Builder))
     return V;
@@ -3516,6 +3510,13 @@ Value *InstCombinerImpl::foldBooleanAndOr(Value *LHS, Value *RHS,
   if (!LHS->getType()->isIntOrIntVectorTy(1))
     return nullptr;
 
+  // handle (roughly):
+  // (icmp ne (A & B), C) | (icmp ne (A & D), E)
+  // (icmp eq (A & B), C) & (icmp eq (A & D), E)
+  if (Value *V = foldLogOpOfMaskedICmps(LHS, RHS, IsAnd, IsLogical, Builder,
+                                        SQ.getWithInstruction(&I)))
+    return V;
+
   if (auto *LHSCmp = dyn_cast<ICmpInst>(LHS))
     if (auto *RHSCmp = dyn_cast<ICmpInst>(RHS))
       if (Value *Res = foldAndOrOfICmps(LHSCmp, RHSCmp, I, IsAnd, IsLogical))
diff --git a/llvm/test/Transforms/InstCombine/eq-of-parts.ll b/llvm/test/Transforms/InstCombine/eq-of-parts.ll
index d07c2e6a5be52..4dc502788db47 100644
--- a/llvm/test/Transforms/InstCombine/eq-of-parts.ll
+++ b/llvm/test/Transforms/InstCombine/eq-of-parts.ll
@@ -1455,10 +1455,8 @@ define i1 @and_trunc_i1(i8 %a1, i8 %a2) {
 define i1 @and_trunc_i1_wrong_const(i8 %a1, i8 %a2) {
 ; CHECK-LABEL: @and_trunc_i1_wrong_const(
 ; CHECK-NEXT:    [[XOR:%.*]] = xor i8 [[A1:%.*]], [[A2:%.*]]
-; CHECK-NEXT:    [[CMP:%.*]] = icmp ult i8 [[XOR]], 4
-; CHECK-NEXT:    [[LOBIT:%.*]] = trunc i8 [[XOR]] to i1
-; CHECK-NEXT:    [[LOBIT_INV:%.*]] = xor i1 [[LOBIT]], true
-; CHECK-NEXT:    [[AND:%.*]] = and i1 [[CMP]], [[LOBIT_INV]]
+; CHECK-NEXT:    [[TMP1:%.*]] = and i8 [[XOR]], -3
+; CHECK-NEXT:    [[AND:%.*]] = icmp eq i8 [[TMP1]], 0
 ; CHECK-NEXT:    ret i1 [[AND]]
 ;
   %xor = xor i8 %a1, %a2
diff --git a/llvm/test/Transforms/InstCombine/onehot_merge.ll b/llvm/test/Transforms/InstCombine/onehot_merge.ll
index e60edc7315d46..33e39d78e1ce4 100644
--- a/llvm/test/Transforms/InstCombine/onehot_merge.ll
+++ b/llvm/test/Transforms/InstCombine/onehot_merge.ll
@@ -1163,10 +1163,9 @@ define i1 @two_types_of_bittest(i8 %x, i8 %c) {
 define i1 @trunc_bittest_and_icmp_bittest(i8 %x, i8 %c) {
 ; CHECK-LABEL: @trunc_bittest_and_icmp_bittest(
 ; CHECK-NEXT:    [[T0:%.*]] = shl nuw i8 1, [[C:%.*]]
-; CHECK-NEXT:    [[TRUNC:%.*]] = trunc i8 [[X:%.*]] to i1
-; CHECK-NEXT:    [[AND:%.*]] = and i8 [[X]], [[T0]]
-; CHECK-NEXT:    [[ICMP2:%.*]] = icmp ne i8 [[AND]], 0
-; CHECK-NEXT:    [[RET:%.*]] = and i1 [[ICMP2]], [[TRUNC]]
+; CHECK-NEXT:    [[TMP1:%.*]] = or i8 [[T0]], 1
+; CHECK-NEXT:    [[TMP2:%.*]] = and i8 [[X:%.*]], [[TMP1]]
+; CHECK-NEXT:    [[RET:%.*]] = icmp eq i8 [[TMP2]], [[TMP1]]
 ; CHECK-NEXT:    ret i1 [[RET]]
 ;
   %t0 = shl i8 1, %c
@@ -1180,11 +1179,9 @@ define i1 @trunc_bittest_and_icmp_bittest(i8 %x, i8 %c) {
 define i1 @trunc_bittest_or_icmp_bittest(i8 %x, i8 %c) {
 ; CHECK-LABEL: @trunc_bittest_or_icmp_bittest(
 ; CHECK-NEXT:    [[T0:%.*]] = shl nuw i8 1, [[C:%.*]]
-; CHECK-NEXT:    [[TRUNC:%.*]] = trunc i8 [[X:%.*]] to i1
-; CHECK-NEXT:    [[AND:%.*]] = and i8 [[X]], [[T0]]
-; CHECK-NEXT:    [[ICMP2:%.*]] = icmp eq i8 [[AND]], 0
-; CHECK-NEXT:    [[NOT:%.*]] = xor i1 [[TRUNC]], true
-; CHECK-NEXT:    [[RET:%.*]] = or i1 [[ICMP2]], [[NOT]]
+; CHECK-NEXT:    [[TMP1:%.*]] = or i8 [[T0]], 1
+; CHECK-NEXT:    [[TMP2:%.*]] = and i8 [[X:%.*]], [[TMP1]]
+; CHECK-NEXT:    [[RET:%.*]] = icmp ne i8 [[TMP2]], [[TMP1]]
 ; CHECK-NEXT:    ret i1 [[RET]]
 ;
   %t0 = shl i8 1, %c

@andjo403
Copy link
Contributor Author

andjo403 commented Feb 26, 2025

what I can see many of the regressions in llvm-opt-benchmark is that icmp ne (and x, 1), 0 or that known bits show that x is only 0 or 1 is not folded to trunc x to i1 but the fold from this PR is one of the reasons for regressions for that fold.

@dtcxzyw
Copy link
Member

dtcxzyw commented Feb 27, 2025

The code change LG. But I don't understand what you mean in the following sentence:

but the fold from this PR is one of the reasons for regressions for that fold.

Do you mean we have to land this patch first before fixing these regressions?

@andjo403
Copy link
Contributor Author

sorry for the bad description what I was trying to say is that some of the regressions that this PR cause eg. https://github.com/dtcxzyw/llvm-opt-benchmark/pull/2173/files#diff-c88279c89225628a242defdd907f1444ed67e844ff858382dbea2edc36237e7fR167035-R167038 and https://github.com/dtcxzyw/llvm-opt-benchmark/pull/2173/files#diff-403632332e00c769093bc90d3e97457d65f8adf3b5824d6cd460e13c166cc164R1747-R1749
is solved by ed57ea5 but the fold from that commit have many regressions and some of those regressions is fixed by this PR.

@andjo403
Copy link
Contributor Author

@dtcxzyw what do you think we shall do with this PR?

@andjo403
Copy link
Contributor Author

this PR also fix/hide the problem in #131824

@dtcxzyw
Copy link
Member

dtcxzyw commented Mar 31, 2025

sorry for the bad description what I was trying to say is that some of the regressions that this PR cause eg. https://github.com/dtcxzyw/llvm-opt-benchmark/pull/2173/files#diff-c88279c89225628a242defdd907f1444ed67e844ff858382dbea2edc36237e7fR167035-R167038 and https://github.com/dtcxzyw/llvm-opt-benchmark/pull/2173/files#diff-403632332e00c769093bc90d3e97457d65f8adf3b5824d6cd460e13c166cc164R1747-R1749 is solved by ed57ea5 but the fold from that commit have many regressions and some of those regressions is fixed by this PR.

This regression is still outstanding: https://alive2.llvm.org/ce/z/4tZwjg
Can you pre-commit this test?

andjo403 added a commit that referenced this pull request Apr 5, 2025
…e(0,2) (NFC)

proof https://alive2.llvm.org/ce/z/xeazCu

this is a regression found in #128861
This fold is done when icmp eq/ne x, 1/0 is folded to trunc nuw x iff x is in the range(0,2)
@andjo403 andjo403 force-pushed the foldLogOpOfMaskedICmpsTrunc branch from dde1040 to 993fff6 Compare April 5, 2025 10:32
@andjo403
Copy link
Contributor Author

andjo403 commented Apr 5, 2025

This regression is still outstanding: https://alive2.llvm.org/ce/z/4tZwjg Can you pre-commit this test?

have added a reduced version of the test in 1657331

llvm-sync bot pushed a commit to arm/arm-toolchain that referenced this pull request Apr 5, 2025
…in the range(0,2) (NFC)

proof https://alive2.llvm.org/ce/z/xeazCu

this is a regression found in llvm/llvm-project#128861
This fold is done when icmp eq/ne x, 1/0 is folded to trunc nuw x iff x is in the range(0,2)
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.

LGTM. The net effect looks positive. I think it is the right direction.

@andjo403 andjo403 merged commit 94f6f03 into llvm:main Apr 9, 2025
9 of 11 checks passed
@andjo403 andjo403 deleted the foldLogOpOfMaskedICmpsTrunc branch April 9, 2025 16:52
AllinLeeYL pushed a commit to AllinLeeYL/llvm-project that referenced this pull request Apr 10, 2025
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.

!struct.x && !struct.y produces worse code than (struct.x == 0) && (struct.y == 0)
3 participants