Skip to content

[ValueTracking] Handle trunc to i1 as condition in dominating condition. #126414

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Feb 11, 2025

Conversation

andjo403
Copy link
Contributor

@andjo403 andjo403 commented Feb 9, 2025

@llvmbot
Copy link
Member

llvmbot commented Feb 9, 2025

@llvm/pr-subscribers-llvm-transforms

@llvm/pr-subscribers-llvm-analysis

Author: Andreas Jonson (andjo403)

Changes

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


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

2 Files Affected:

  • (modified) llvm/lib/Analysis/ValueTracking.cpp (+19)
  • (modified) llvm/test/Transforms/InstCombine/known-bits.ll (+10-20)
diff --git a/llvm/lib/Analysis/ValueTracking.cpp b/llvm/lib/Analysis/ValueTracking.cpp
index 8a9ad55366ee703..40fae7ca95d774b 100644
--- a/llvm/lib/Analysis/ValueTracking.cpp
+++ b/llvm/lib/Analysis/ValueTracking.cpp
@@ -801,6 +801,22 @@ static void computeKnownBitsFromCond(const Value *V, Value *Cond,
 
   if (auto *Cmp = dyn_cast<ICmpInst>(Cond))
     computeKnownBitsFromICmpCond(V, Cmp, Known, SQ, Invert);
+
+  if (match(Cond, m_Not(m_Value(Cond))))
+    Invert = !Invert;
+
+  if (match(Cond, m_Trunc(m_Specific(V)))) {
+    KnownBits DstKnown(1);
+    if (Invert) {
+      DstKnown.setAllZero();
+    } else {
+      DstKnown.setAllOnes();
+    }
+    if (cast<TruncInst>(Cond)->hasNoUnsignedWrap())
+      Known = Known.unionWith(DstKnown.zext(Known.getBitWidth()));
+    else
+      Known = Known.unionWith(DstKnown.anyext(Known.getBitWidth()));
+  }
 }
 
 void llvm::computeKnownBitsFromContext(const Value *V, KnownBits &Known,
@@ -10272,6 +10288,9 @@ void llvm::findValuesAffectedByCondition(
                                                            m_Value()))) {
       // Handle patterns that computeKnownFPClass() support.
       AddAffected(A);
+    } else if (!IsAssume && match(V, m_CombineOr(m_Trunc(m_Value(X)),
+                                                 m_Not(m_Trunc(m_Value(X)))))) {
+      AddAffected(X);
     }
   }
 }
diff --git a/llvm/test/Transforms/InstCombine/known-bits.ll b/llvm/test/Transforms/InstCombine/known-bits.ll
index a3872fefecf3b31..b47c3de21f9feaf 100644
--- a/llvm/test/Transforms/InstCombine/known-bits.ll
+++ b/llvm/test/Transforms/InstCombine/known-bits.ll
@@ -2167,11 +2167,9 @@ define i8 @test_trunc_and_1(i8 %a) {
 ; CHECK-NEXT:    [[CAST:%.*]] = trunc i8 [[A:%.*]] to i1
 ; CHECK-NEXT:    br i1 [[CAST]], label [[IF_THEN:%.*]], label [[IF_ELSE:%.*]]
 ; CHECK:       if.then:
-; CHECK-NEXT:    [[B:%.*]] = and i8 [[A]], 1
-; CHECK-NEXT:    ret i8 [[B]]
+; CHECK-NEXT:    ret i8 1
 ; CHECK:       if.else:
-; CHECK-NEXT:    [[C:%.*]] = and i8 [[A]], 1
-; CHECK-NEXT:    ret i8 [[C]]
+; CHECK-NEXT:    ret i8 0
 ;
 entry:
   %cast = trunc i8 %a to i1
@@ -2192,11 +2190,9 @@ define i8 @test_not_trunc_and_1(i8 %a) {
 ; CHECK-NEXT:    [[CAST:%.*]] = trunc i8 [[A:%.*]] to i1
 ; CHECK-NEXT:    br i1 [[CAST]], label [[IF_ELSE:%.*]], label [[IF_THEN:%.*]]
 ; CHECK:       if.then:
-; CHECK-NEXT:    [[B:%.*]] = and i8 [[A]], 1
-; CHECK-NEXT:    ret i8 [[B]]
+; CHECK-NEXT:    ret i8 0
 ; CHECK:       if.else:
-; CHECK-NEXT:    [[C:%.*]] = and i8 [[A]], 1
-; CHECK-NEXT:    ret i8 [[C]]
+; CHECK-NEXT:    ret i8 1
 ;
 entry:
   %cast = trunc i8 %a to i1
@@ -2243,11 +2239,9 @@ define i8 @test_trunc_nuw_and_1(i8 %a) {
 ; CHECK-NEXT:    [[CAST:%.*]] = trunc nuw i8 [[A:%.*]] to i1
 ; CHECK-NEXT:    br i1 [[CAST]], label [[IF_ELSE:%.*]], label [[IF_THEN:%.*]]
 ; CHECK:       if.then:
-; CHECK-NEXT:    [[B:%.*]] = and i8 [[A]], 1
-; CHECK-NEXT:    ret i8 [[B]]
+; CHECK-NEXT:    ret i8 0
 ; CHECK:       if.else:
-; CHECK-NEXT:    [[C:%.*]] = and i8 [[A]], 1
-; CHECK-NEXT:    ret i8 [[C]]
+; CHECK-NEXT:    ret i8 1
 ;
 entry:
   %cast = trunc nuw i8 %a to i1
@@ -2268,11 +2262,9 @@ define i8 @test_trunc_nuw_or_2(i8 %a) {
 ; CHECK-NEXT:    [[CAST:%.*]] = trunc nuw i8 [[A:%.*]] to i1
 ; CHECK-NEXT:    br i1 [[CAST]], label [[IF_ELSE:%.*]], label [[IF_THEN:%.*]]
 ; CHECK:       if.then:
-; CHECK-NEXT:    [[B:%.*]] = or i8 [[A]], 2
-; CHECK-NEXT:    ret i8 [[B]]
+; CHECK-NEXT:    ret i8 2
 ; CHECK:       if.else:
-; CHECK-NEXT:    [[C:%.*]] = or i8 [[A]], 2
-; CHECK-NEXT:    ret i8 [[C]]
+; CHECK-NEXT:    ret i8 3
 ;
 entry:
   %cast = trunc nuw i8 %a to i1
@@ -2293,11 +2285,9 @@ define i8 @test_not_trunc_nuw_and_1(i8 %a) {
 ; CHECK-NEXT:    [[CAST:%.*]] = trunc nuw i8 [[A:%.*]] to i1
 ; CHECK-NEXT:    br i1 [[CAST]], label [[IF_ELSE:%.*]], label [[IF_THEN:%.*]]
 ; CHECK:       if.then:
-; CHECK-NEXT:    [[B:%.*]] = and i8 [[A]], 1
-; CHECK-NEXT:    ret i8 [[B]]
+; CHECK-NEXT:    ret i8 0
 ; CHECK:       if.else:
-; CHECK-NEXT:    [[C:%.*]] = and i8 [[A]], 1
-; CHECK-NEXT:    ret i8 [[C]]
+; CHECK-NEXT:    ret i8 1
 ;
 entry:
   %cast = trunc nuw i8 %a to i1

@@ -801,6 +801,22 @@ static void computeKnownBitsFromCond(const Value *V, Value *Cond,

if (auto *Cmp = dyn_cast<ICmpInst>(Cond))
computeKnownBitsFromICmpCond(V, Cmp, Known, SQ, Invert);

if (match(Cond, m_Not(m_Value(Cond))))
Invert = !Invert;
Copy link
Contributor

Choose a reason for hiding this comment

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

Not in a br cond will be canonicalized away by swapping successors. Shouldn't need this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

notice at least some regressions in opt-benchmark when I removed it,
one example I think is this pattern:

  %630 = trunc i8 %629 to i1
  %not. = xor i1 %630, true
  %.0215.shrunk. = or i1 %spec.select472, %not.
  br i1 %.0215.shrunk., label %632, label %.thread

so I shall maybe try to make a test that cover this case?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

added the @test_not_trunc_cond_and that do not pass without this.

Copy link
Contributor

Choose a reason for hiding this comment

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

Can you please split this off into a followup change? I think it makes sense to handle it, but if we do so, we should do it generically, not for trunc conditions in particular.

@andjo403 andjo403 force-pushed the truncCondInDominatingCond branch from a684f10 to d148f62 Compare February 9, 2025 12:59
@goldsteinn
Copy link
Contributor

What are your proofs proving? The tgt and src seem to be duplicated...

DstKnown.setAllOnes();
}
if (cast<TruncInst>(Cond)->hasNoUnsignedWrap())
Known = Known.unionWith(DstKnown.zext(Known.getBitWidth()));
Copy link
Contributor

Choose a reason for hiding this comment

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

you can sext if you have trunc nsw

Copy link
Contributor Author

Choose a reason for hiding this comment

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

avoided sext as it was not handled i #125414 seems like it is uncommon to have sext for trunc to i1 only found 3 in llvm-opt-benchmark

@andjo403
Copy link
Contributor Author

andjo403 commented Feb 9, 2025

fixed the proofs copy past is good when looking at what is copied.

@@ -10272,6 +10288,9 @@ void llvm::findValuesAffectedByCondition(
m_Value()))) {
// Handle patterns that computeKnownFPClass() support.
AddAffected(A);
} else if (!IsAssume && match(V, m_CombineOr(m_Trunc(m_Value(X)),
Copy link
Member

Choose a reason for hiding this comment

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

Is !IsAssume used to avoid issues with ephemeral values? If so, please add some comments here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

partly, for the not #126423 (comment) it is the avoid issues with ephemeral values.
but also to avoid to add the same value again as truncated value of V is already added for assume above as the AddAffected Looks through trunc.
will add comment

@andjo403 andjo403 force-pushed the truncCondInDominatingCond branch from d148f62 to 167513c Compare February 10, 2025 18:33
@andjo403
Copy link
Contributor Author

rebased on top of #126423 so ready for review again

Copy link
Contributor

@nikic nikic left a comment

Choose a reason for hiding this comment

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

LGTM

@andjo403 andjo403 merged commit cf87eb9 into llvm:main Feb 11, 2025
8 checks passed
joaosaffran pushed a commit to joaosaffran/llvm-project that referenced this pull request Feb 14, 2025
@andjo403 andjo403 deleted the truncCondInDominatingCond branch February 16, 2025 12:14
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