[ValueTracking] Convert `isKnownNonZero` to use SimplifyQuery #85863

dtcxzyw · 2024-03-19T20:22:43Z

This patch converts isKnownNonZero to use SimplifyQuery. Then we can use the context information from DomCondCache.

Fixes #85823.
Alive2: https://alive2.llvm.org/ce/z/QUvHVj

llvmbot · 2024-03-19T20:23:15Z

@llvm/pr-subscribers-clang
@llvm/pr-subscribers-clang-codegen
@llvm/pr-subscribers-llvm-analysis
@llvm/pr-subscribers-compiler-rt-sanitizer

@llvm/pr-subscribers-llvm-transforms

Author: Yingwei Zheng (dtcxzyw)

Changes

Fixes #85823.
Alive2: https://alive2.llvm.org/ce/z/QUvHVj

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

2 Files Affected:

(modified) llvm/lib/Transforms/InstCombine/InstCombineCompares.cpp (+6-3)
(modified) llvm/test/Transforms/InstCombine/icmp-dom.ll (+85)

diff --git a/llvm/lib/Transforms/InstCombine/InstCombineCompares.cpp b/llvm/lib/Transforms/InstCombine/InstCombineCompares.cpp
index 0dce0077bf1588..7cb5e98dea2581 100644
--- a/llvm/lib/Transforms/InstCombine/InstCombineCompares.cpp
+++ b/llvm/lib/Transforms/InstCombine/InstCombineCompares.cpp
@@ -1771,11 +1771,14 @@ Instruction *InstCombinerImpl::foldICmpAndConstConst(ICmpInst &Cmp,
       return new ICmpInst(NewPred, X, Zero);
     }
 
-    APInt NewC2 = *C2;
     KnownBits Know = computeKnownBits(And->getOperand(0), 0, And);
+    if (Know.One.intersects(*C2))
+      return replaceInstUsesWith(
+          Cmp, ConstantInt::getBool(Cmp.getType(), isICMP_NE));
+
     // Set high zeros of C2 to allow matching negated power-of-2.
-    NewC2 = *C2 | APInt::getHighBitsSet(C2->getBitWidth(),
-                                        Know.countMinLeadingZeros());
+    APInt NewC2 = *C2 | APInt::getHighBitsSet(C2->getBitWidth(),
+                                              Know.countMinLeadingZeros());
 
     // Restrict this fold only for single-use 'and' (PR10267).
     // ((%x & C) == 0) --> %x u< (-C)  iff (-C) is power of two.
diff --git a/llvm/test/Transforms/InstCombine/icmp-dom.ll b/llvm/test/Transforms/InstCombine/icmp-dom.ll
index f4b9022d14349b..d97a2e707ae235 100644
--- a/llvm/test/Transforms/InstCombine/icmp-dom.ll
+++ b/llvm/test/Transforms/InstCombine/icmp-dom.ll
@@ -403,3 +403,88 @@ truelabel:
 falselabel:
   ret i8 0
 }
+
+define i1 @and_mask1_eq(i32 %conv) {
+; CHECK-LABEL: @and_mask1_eq(
+; CHECK-NEXT:  entry:
+; CHECK-NEXT:    [[AND:%.*]] = and i32 [[CONV:%.*]], 1
+; CHECK-NEXT:    [[CMP:%.*]] = icmp eq i32 [[AND]], 0
+; CHECK-NEXT:    br i1 [[CMP]], label [[THEN:%.*]], label [[ELSE:%.*]]
+; CHECK:       then:
+; CHECK-NEXT:    ret i1 false
+; CHECK:       else:
+; CHECK-NEXT:    call void @dummy()
+; CHECK-NEXT:    ret i1 false
+;
+entry:
+  %and = and i32 %conv, 1
+  %cmp = icmp eq i32 %and, 0
+  br i1 %cmp, label %then, label %else
+
+then:
+  ret i1 0
+
+else:
+  call void @dummy()
+  %and1 = and i32 %conv, 3
+  %cmp1 = icmp eq i32 %and1, 0
+  ret i1 %cmp1
+}
+
+define i1 @and_mask1_ne(i32 %conv) {
+; CHECK-LABEL: @and_mask1_ne(
+; CHECK-NEXT:  entry:
+; CHECK-NEXT:    [[AND:%.*]] = and i32 [[CONV:%.*]], 1
+; CHECK-NEXT:    [[CMP:%.*]] = icmp eq i32 [[AND]], 0
+; CHECK-NEXT:    br i1 [[CMP]], label [[THEN:%.*]], label [[ELSE:%.*]]
+; CHECK:       then:
+; CHECK-NEXT:    ret i1 false
+; CHECK:       else:
+; CHECK-NEXT:    call void @dummy()
+; CHECK-NEXT:    ret i1 true
+;
+entry:
+  %and = and i32 %conv, 1
+  %cmp = icmp eq i32 %and, 0
+  br i1 %cmp, label %then, label %else
+
+then:
+  ret i1 0
+
+else:
+  call void @dummy()
+  %and1 = and i32 %conv, 3
+  %cmp1 = icmp ne i32 %and1, 0
+  ret i1 %cmp1
+}
+
+define i1 @and_mask2(i32 %conv) {
+; CHECK-LABEL: @and_mask2(
+; CHECK-NEXT:  entry:
+; CHECK-NEXT:    [[AND:%.*]] = and i32 [[CONV:%.*]], 4
+; CHECK-NEXT:    [[CMP:%.*]] = icmp eq i32 [[AND]], 0
+; CHECK-NEXT:    br i1 [[CMP]], label [[THEN:%.*]], label [[ELSE:%.*]]
+; CHECK:       then:
+; CHECK-NEXT:    ret i1 false
+; CHECK:       else:
+; CHECK-NEXT:    call void @dummy()
+; CHECK-NEXT:    [[AND1:%.*]] = and i32 [[CONV]], 3
+; CHECK-NEXT:    [[CMP1:%.*]] = icmp eq i32 [[AND1]], 0
+; CHECK-NEXT:    ret i1 [[CMP1]]
+;
+entry:
+  %and = and i32 %conv, 4
+  %cmp = icmp eq i32 %and, 0
+  br i1 %cmp, label %then, label %else
+
+then:
+  ret i1 0
+
+else:
+  call void @dummy()
+  %and1 = and i32 %conv, 3
+  %cmp1 = icmp eq i32 %and1, 0
+  ret i1 %cmp1
+}
+
+declare void @dummy()

PR Link: llvm/llvm-project#85863

llvm/test/Transforms/InstCombine/icmp-dom.ll

llvm/lib/Transforms/InstCombine/InstCombineCompares.cpp

dtcxzyw · 2024-03-20T10:25:47Z

Oh, simplifyICmpWithZero in InstSimplify should handle this.

llvm-project/llvm/lib/Analysis/InstructionSimplify.cpp

Lines 2967 to 2984 in 7227ec9

    
           /// Try hard to fold icmp with zero RHS because this is a common case. 
        
           static Value *simplifyICmpWithZero(CmpInst::Predicate Pred, Value *LHS, 
        
                                              Value *RHS, const SimplifyQuery &Q) { 
        
             if (!match(RHS, m_Zero())) 
        
               return nullptr; 
        
             Type *ITy = getCompareTy(LHS); // The return type. 
        
             switch (Pred) { 
        
             default: 
        
               llvm_unreachable("Unknown ICmp predicate!"); 
        
             case ICmpInst::ICMP_ULT: 
        
               return getFalse(ITy); 
        
             case ICmpInst::ICMP_UGE: 
        
               return getTrue(ITy); 
        
             case ICmpInst::ICMP_EQ: 
        
             case ICmpInst::ICMP_ULE: 
        
               if (isKnownNonZero(LHS, Q.DL, 0, Q.AC, Q.CxtI, Q.DT, Q.IIQ.UseInstrInfo)) 
        
                 return getFalse(ITy);

I will fix it later.

PR Link: llvm/llvm-project#85863

nikic · 2024-03-21T14:33:21Z

Can you please fix the clang build?

nikic · 2024-03-21T14:34:51Z

llvm/lib/Analysis/LazyValueInfo.cpp

@@ -641,7 +641,7 @@ LazyValueInfoImpl::solveBlockValueImpl(Value *Val, BasicBlock *BB) {
  // instruction is placed, even if it could legally be hoisted much higher.
  // That is unfortunate.
  PointerType *PT = dyn_cast<PointerType>(BBI->getType());
-  if (PT && isKnownNonZero(BBI, DL))
+  if (PT && isKnownNonZero(BBI, /*Depth=*/0, SimplifyQuery(DL)))


FWIW, you could just pass DL here and other places that use only DL. It will be converted implicitly. But this is fine.

dtcxzyw · 2024-03-21T15:18:41Z

Can you please fix the clang build?

Done.

dtcxzyw · 2024-04-11T14:14:56Z

Ping.

goldsteinn · 2024-04-11T15:19:57Z

llvm/include/llvm/Analysis/ValueTracking.h

-                    AssumptionCache *AC = nullptr,
-                    const Instruction *CxtI = nullptr,
-                    const DominatorTree *DT = nullptr,
-                    bool UseInstrInfo = true);


Do you also need to erase the old API? computeKnownBits has both.

I don't see any reason to keep the old API. It is unlikely to break LLVM downstream users' build (except some static analysis tools).

There are a few places it makes the code simpler (i.e the cases with just DL and depth==0).

llvm/lib/CodeGen/CodeGenPrepare.cpp

llvm/lib/Analysis/InstructionSimplify.cpp

goldsteinn · 2024-04-11T15:29:32Z

llvm/test/Transforms/InstCombine/icmp-dom.ll

+  %and1 = and i32 %conv, 7
+  %cmp1 = icmp eq i32 %and1, 0
+  ret i1 %cmp1
+}


Would be nicer if this where NFC...

goldsteinn · 2024-04-11T15:54:09Z

LGTM. Wait on nikic

nikic · 2024-04-11T23:34:24Z

Looks like the clang build is failing again?

nikic · 2024-04-11T23:36:17Z

llvm/lib/Analysis/LazyValueInfo.cpp

@@ -645,7 +645,7 @@ LazyValueInfoImpl::solveBlockValueImpl(Value *Val, BasicBlock *BB) {
  // instruction is placed, even if it could legally be hoisted much higher.
  // That is unfortunate.
  PointerType *PT = dyn_cast<PointerType>(BBI->getType());
-  if (PT && isKnownNonZero(BBI, DL))
+  if (PT && isKnownNonZero(BBI, /*Depth=*/0, DL))


Side note: We're currently inconsistent about this, but I think it would be best if we moved all the depth parameters after SQ, because end-users of the API don't need to specify depth.

If you aren't going to implement that, I will after this patch lands.

The fact that this PR changes it so that isKnownNonZero(BBI, DL) is valid in LLVM 18, used to be valid in LLVM 19, is currently an error in LLVM 19, but will become valid in LLVM 19 again is unfortunate and difficult to account for in downstream projects. @goldsteinn If you are going to change that, do you think it is worth updating downstream code to support the current API, or is that change going to go in fast enough that we can just wait?

If I don't make the llvm19 cutoff, I'll drop it.

Thanks, I had been slightly optimistic that this was just going to be a few days, but it sounds like it's going to be longer. In that case I will check what is easiest for us (which might be to submit that LLVM change ourselves).

Created #88873 for it.

Thank you for making the PR :)

dtcxzyw · 2024-04-12T07:58:33Z

Looks like the clang build is failing again?

Done.

nikic

LG

Prior to #85863, the required parameters of llvm::isKnownNonZero were Value and DataLayout. After, they are Value, Depth, and SimplifyQuery, where SimplifyQuery is implicitly constructible from DataLayout. The change to move Depth before SimplifyQuery needed callers to be updated unnecessarily, and as commented in #85863, we actually want Depth to be after SimplifyQuery anyway so that it can be defaulted and the caller does not need to specify it.

dtcxzyw requested a review from goldsteinn March 19, 2024 20:22

dtcxzyw requested a review from nikic as a code owner March 19, 2024 20:22

llvmbot added the llvm:transforms label Mar 19, 2024

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

pre-commit: test PR85863

6fe0089

PR Link: llvm/llvm-project#85863

dtcxzyw mentioned this pull request Mar 19, 2024

pre-commit: test PR85863 dtcxzyw/llvm-opt-benchmark#429

Open

dtcxzyw changed the title ~~[InstCombine] Fold icmp eq/ne (X and C), 0 when partial bits are known~~ [InstCombine] Fold icmp eq/ne (X and C), 0 when partial bits are known Mar 19, 2024

nikic reviewed Mar 19, 2024

View reviewed changes

llvm/test/Transforms/InstCombine/icmp-dom.ll Outdated Show resolved Hide resolved

llvm/test/Transforms/InstCombine/icmp-dom.ll Outdated Show resolved Hide resolved

goldsteinn reviewed Mar 19, 2024

View reviewed changes

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

dtcxzyw force-pushed the perf/icmp-and-c-zero branch from fd6783e to f3c73ba Compare March 21, 2024 13:22

llvmbot added vectorization compiler-rt:sanitizer llvm:analysis labels Mar 21, 2024

dtcxzyw changed the title ~~[InstCombine] Fold icmp eq/ne (X and C), 0 when partial bits are known~~ [ValueTracking] Convert isKnownNonZero to use SimplifyQuery Mar 21, 2024

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

pre-commit: test PR85863

93e57e8

PR Link: llvm/llvm-project#85863

nikic reviewed Mar 21, 2024

View reviewed changes

dtcxzyw force-pushed the perf/icmp-and-c-zero branch from f3c73ba to 746f3cc Compare March 21, 2024 15:18

llvmbot added clang Clang issues not falling into any other category clang:codegen labels Mar 21, 2024

dtcxzyw force-pushed the perf/icmp-and-c-zero branch from 746f3cc to 3289593 Compare April 11, 2024 14:14

goldsteinn reviewed Apr 11, 2024

View reviewed changes

llvm/lib/CodeGen/CodeGenPrepare.cpp Outdated Show resolved Hide resolved

goldsteinn reviewed Apr 11, 2024

View reviewed changes

llvm/lib/Analysis/InstructionSimplify.cpp Show resolved Hide resolved

goldsteinn reviewed Apr 11, 2024

View reviewed changes

nikic reviewed Apr 11, 2024

View reviewed changes

dtcxzyw added 4 commits April 12, 2024 15:41

[ValueTracking] Add pre-commit tests. NFC.

9b725ff

[ValueTracking] Convert isKnownNonZero to use SimplifyQuery

b80b7de

[ValueTracking] Address review comments

4af6856

[ValueTracking] Fix clang build failure

e780f79

dtcxzyw force-pushed the perf/icmp-and-c-zero branch from 6eba984 to e780f79 Compare April 12, 2024 07:58

nikic approved these changes Apr 12, 2024

View reviewed changes

dtcxzyw merged commit e0a6287 into llvm:main Apr 12, 2024
4 checks passed

dtcxzyw deleted the perf/icmp-and-c-zero branch April 12, 2024 15:47

hvdijk mentioned this pull request Apr 16, 2024

[ValueTracking] Restore isKnownNonZero parameter order. #88873

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[ValueTracking] Convert `isKnownNonZero` to use SimplifyQuery #85863

[ValueTracking] Convert `isKnownNonZero` to use SimplifyQuery #85863

dtcxzyw commented Mar 19, 2024 •

edited

llvmbot commented Mar 19, 2024 •

edited

dtcxzyw commented Mar 20, 2024

nikic commented Mar 21, 2024

nikic Mar 21, 2024

dtcxzyw commented Mar 21, 2024

dtcxzyw commented Apr 11, 2024

goldsteinn Apr 11, 2024

dtcxzyw Apr 11, 2024

goldsteinn Apr 11, 2024

goldsteinn Apr 11, 2024

goldsteinn commented Apr 11, 2024

nikic commented Apr 11, 2024

nikic Apr 11, 2024

goldsteinn Apr 12, 2024

hvdijk Apr 15, 2024

goldsteinn Apr 15, 2024

hvdijk Apr 16, 2024

hvdijk Apr 16, 2024

goldsteinn Apr 16, 2024

dtcxzyw commented Apr 12, 2024

nikic left a comment

[ValueTracking] Convert isKnownNonZero to use SimplifyQuery #85863

[ValueTracking] Convert isKnownNonZero to use SimplifyQuery #85863

Conversation

dtcxzyw commented Mar 19, 2024 • edited

llvmbot commented Mar 19, 2024 • edited

dtcxzyw commented Mar 20, 2024

nikic commented Mar 21, 2024

Choose a reason for hiding this comment

dtcxzyw commented Mar 21, 2024

dtcxzyw commented Apr 11, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

goldsteinn commented Apr 11, 2024

nikic commented Apr 11, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dtcxzyw commented Apr 12, 2024

nikic left a comment

Choose a reason for hiding this comment

[ValueTracking] Convert `isKnownNonZero` to use SimplifyQuery #85863

[ValueTracking] Convert `isKnownNonZero` to use SimplifyQuery #85863

dtcxzyw commented Mar 19, 2024 •

edited

llvmbot commented Mar 19, 2024 •

edited