Skip to content
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

bv_decide - kernel deterministic timeout + ofBool normalization #6043

Closed
3 tasks done
tobiasgrosser opened this issue Nov 12, 2024 · 3 comments · Fixed by #6453
Closed
3 tasks done

bv_decide - kernel deterministic timeout + ofBool normalization #6043

tobiasgrosser opened this issue Nov 12, 2024 · 3 comments · Fixed by #6453
Labels
bug Something isn't working P-medium We may work on this issue if we find the time

Comments

@tobiasgrosser
Copy link
Contributor

Prerequisites

Please put an X between the brackets as you perform the following steps:

Description

The following test case fails with 'kernel deterministic timeout)
import Lean.Elab.Tactic.BVDecide

/-
(kernel) deterministic timeout
-/
theorem extracted_1 (x : BitVec 32) :
  BitVec.ofBool (x != 51#32) &&& BitVec.ofBool (x != 50#32) =
    BitVec.ofBool (4294967294#32 > x + 4294967244#32) := by
  -- simp [BitVec.ofBool_and_ofBool, BitVec.ofBool_eq_iff_eq]
  bv_decide

Steps to Reproduce

  1. Copy test case into lean
  2. Observe error

Expected behavior: bv_decide should solve this goal quickly. In case it fails, it should not be a kernel timeout

Actual behavior: A kernel timeout

Versions

Lean 4.15.0-nightly-2024-11-11
Target: arm64-apple-darwin23.6.0 macOS

Additional Information

This bug goes away if I simplify with the relevant ofBool_* theorems. Adding them to bv_normalize does not help, though.

Impact

Add 👍 to issues you consider important. If others are impacted by this issue, please ask them to add 👍 to it.

@tobiasgrosser tobiasgrosser added the bug Something isn't working label Nov 12, 2024
@hargoniX
Copy link
Contributor

The error is correct but the diagnosis around BitVec.ofBool is wrong. bv_decide reduces ofBool to ite and the bug also exists if the ofBool/ite gets hidden behind variables + equality statements. So this is not just something we can fix by adding more simp lemmas surrounding ofBool or ite. It's a more fundamental issue.

@tobiasgrosser
Copy link
Contributor Author

tobiasgrosser commented Nov 12, 2024

The error is correct but the diagnosis around BitVec.ofBool is wrong. bv_decide reduces ofBool to ite and the bug also exists if the ofBool/ite gets hidden behind variables + equality statements. So this is not just something we can fix by adding more simp lemmas surrounding ofBool or ite. It's a more fundamental issue.

AFAIU there are two issues in this test case. One is the fundamental issue with 'deterministic timeout'. The second is that bv_normalize should normalize this better.

In particular, we currently get:

theorem extracted_1 (x : BitVec 32) :
  BitVec.ofBool (x != 51#32) &&& BitVec.ofBool (x != 50#32) =
    BitVec.ofBool (4294967294#32 > x + 4294967244#32) := by
  
  /-
  case h
  x : BitVec 32
  a✝ : (!((if (!x == 51#32) = true then 1#1 else 0#1) &&& if (!x == 50#32) = true then 1#1 else 0#1) ==
        if (x + 4294967244#32).ult 4294967294#32 = true then 1#1 else 0#1) =
    true
  ⊢ False
  -/
  bv_normalize

but should get

theorem extracted_1 (x : BitVec 32) :
  BitVec.ofBool (x != 51#32) &&& BitVec.ofBool (x != 50#32) =
    BitVec.ofBool (4294967294#32 > x + 4294967244#32) := by
  simp [BitVec.ofBool_and_ofBool, BitVec.ofBool_eq_iff_eq]
  /-
  case h
  x : BitVec 32
  a✝ : (!(!x == 51#32 && !x == 50#32) == (x + 4294967244#32).ult 4294967294#32) = true
  ⊢ False
  -/
  bv_normalize
  bv_decide

This improved bv_normalize would hide the 'deterministic timeout' bug but is generally desirable.

Or is there a reason we would want to bit-blast an if-statement if there is a way to eliminate it?

@hargoniX
Copy link
Contributor

There is almost no difference between the bitblasted versions of these two, they get solved in basically the same time. The way to rewrite this would be with rewrite rules on the ite form so we actually handle not only BitVec.ofBool but all ite problems of this form. But even then you can trivially retrigger the itmeout by hiding the BitVec.ofBool behind variables so the actual issue at hand here is not solved by rewrites.

@leanprover-bot leanprover-bot added the P-medium We may work on this issue if we find the time label Nov 15, 2024
github-merge-queue bot pushed a commit that referenced this issue Dec 26, 2024
This PR improves bv_decide's performance in the presence of large
literals.

The core change of this PR is the reformulation of the reflection code
for literals to:
```diff
 def eval (assign : Assignment) : BVExpr w → BitVec w
   | .var idx =>
-    let ⟨bv⟩ := assign.get idx
-    bv.truncate w
+    let packedBv := assign.get idx
+    /-
+    This formulation improves performance, as in a well formed expression the condition always holds
+    so there is no need for the more involved `BitVec.truncate` logic.
+    -/
+    if h : packedBv.w = w then
+      h ▸ packedBv.bv
+    else
+      packedBv.bv.truncate w
```
The remainder is merely further simplifications that make the terms
smaller and easier to deal with in general. This change is motivated by
applying the following diff to the kernel:
```diff
diff --git a/src/kernel/type_checker.cpp b/src/kernel/type_checker.cpp
index b0e6844dca..f13bb96bd4 100644
--- a/src/kernel/type_checker.cpp
+++ b/src/kernel/type_checker.cpp
@@ -518,6 +518,7 @@ optional<constant_info> type_checker::is_delta(expr const & e) const {
 optional<expr> type_checker::unfold_definition_core(expr const & e) {
     if (is_constant(e)) {
         if (auto d = is_delta(e)) {
+//            std::cout << "Working on unfolding: " << d->get_name() << std::endl;
             if (length(const_levels(e)) == d->get_num_lparams()) {
                 if (m_diag) {
                     m_diag->record_unfold(d->get_name());
```
and observing that in the test case from #6043 we see a long series of
```
Working on unfolding: Bool.decEq
Working on unfolding: Bool.decEq.match_1
Working on unfolding: Bool.casesOn
Working on unfolding: Nat.ble
Working on unfolding: Nat.brecOn
Working on unfolding: Nat.beq.match_1
Working on unfolding: Nat.casesOn
Working on unfolding: Nat.casesOn
Working on unfolding: Nat.beq.match_1
Working on unfolding: Nat.casesOn
Working on unfolding: Nat.casesOn
```
the chain begins with `BitVec.truncate`, works through a few
abstractions and then continues like above forever, so I avoid the call
to truncate like this. It is not quite clear to me why removing `ofBool`
helps so much here, maybe some other kernel heuristic kicks in to rescue
us.

Either way this diff is a general improvement for reflection of `BitVec`
constants as we should never have to run `BitVec.truncate` again!

Fixes: #6043
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working P-medium We may work on this issue if we find the time
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants