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

simp hang on simplifying: build a structure, followed by a projection #4413

Closed
bollu opened this issue Jun 10, 2024 · 3 comments · Fixed by #4421
Closed

simp hang on simplifying: build a structure, followed by a projection #4413

bollu opened this issue Jun 10, 2024 · 3 comments · Fixed by #4421
Labels
bug Something isn't working

Comments

@bollu
Copy link
Contributor

bollu commented Jun 10, 2024

#eval Lean.versionString -- "4.9.0-nightly-2024-05-23"

structure Note where
  pitch : UInt64
  start : Nat

def Note.containsNote (n1 n2 : Note) : Prop :=
  n1.start ≤ n2.start

def Note.lowerSemitone (n : Note) : Note :=
  { n with pitch := n.pitch - 1 }

theorem Note.self_containsNote_lowerSemitone_self (n : Note) :
    n.containsNote (Note.lowerSemitone n) := by
  -- simp [Note.containsNote, Note.lowerSemitone] -- uncomment to see Lean hang
  sorry
@bollu bollu added the bug Something isn't working label Jun 10, 2024
@kmill
Copy link
Collaborator

kmill commented Jun 10, 2024

@bollu also reported that dsimp can hang here:

theorem Note.self_containsNote_lowerSemitone_self (n : Note) :
    n.containsNote (Note.lowerSemitone n) := by
  -- dsimp (config := { proj := false }) [Note.containsNote, Note.lowerSemitone] -- does not hang
  dsimp (config := { proj := true })[Note.containsNote, Note.lowerSemitone] -- hangs.

@llllvvuu
Copy link
Contributor

llllvvuu commented Jun 10, 2024

bisection: this is broken back to 4.0.0

@llllvvuu
Copy link
Contributor

llllvvuu commented Jun 10, 2024

It is due to unfolding UInt64 subtraction = Fin subtraction. Here is a minimization:

structure Note where
  pitch : UInt32
  start : Nat

def Note.lowerSemitone (n : Note) : Note :=
  { n with pitch := n.pitch - 0 }

set_option maxRecDepth 100 in
set_option maxHeartbeats 1000 in
theorem Note.self_containsNote_lowerSemitone_self (n : Note) :
    Nat.zero ≤ n.lowerSemitone.start := by
  dsimp (config := { proj := true }) only [Note.lowerSemitone]
  exact Nat.zero_le n.start

The elaboration completes and returns:

fun (n : Note) => id.{0} (LE.le.{0} Nat instLENat Nat.zero (Note.start (Note.lowerSemitone n))) (Nat.zero_le (Note.start n))

it seems to be the type checking which is slow, as it is unfolding Fin subtraction - 0 to + 65535 and then succ 65535 times.

Another variant:

structure Note where
  pitch : UInt64
  start : Nat

def lowerSemitone := fun (n : Note) => Note.mk (n.1 - 0) n.2
-- same issue:
-- def lowerSemitone := fun (n : Note) => Note.mk (n.1 + 100) n.2

set_option maxRecDepth 100 in
theorem Note.self_containsNote_lowerSemitone_self (n : Note) :
    0 ≤ (lowerSemitone n).start :=
  (Nat.zero_le (Note.start n))

This one never finishes elaborating.

But there is no issue if the def is inlined into the statement:

-- doesn't hang
theorem Note.self_containsNote_lowerSemitone_self (n : Note) :
    0 ≤ ((fun (n : Note) => Note.mk (n.1 - 0) n.2) n).start :=
  (Nat.zero_le (Note.start n))

-- also doesn't hang
theorem Note.self_containsNote_lowerSemitone_self (n : Note) :
    let lowerSemitone := fun (n : Note) => Note.mk (n.1 - 0) n.2
    0 ≤ (lowerSemitone n).start :=
  (Nat.zero_le (Note.start n))

leodemoura added a commit that referenced this issue Jun 11, 2024
The performance issue at #4413 is due to our `Fin.sub` definition.
```
def sub : Fin n → Fin n → Fin n
  | ⟨a, h⟩, ⟨b, _⟩ => ⟨(a + (n - b)) % n, mlt h⟩
```
Thus, the following runs out of stack space
```
example (a : UInt64) : a - 1 = a :=
  rfl
```
at the `isDefEq` test
```
(a.val.val + 18446744073709551615) % 18446744073709551616 =?= a.val.val
```

From the user's perspective, this timeout is unexpected since they are
using small numerals, and none of the other `Fin` basic operations
(such as `Fin.add` and `Fin.mul`) suffer from this problem.

This PR implements an inelegant solution for the performance issue. It
redefines `Fin.sub` as
```
def sub : Fin n → Fin n → Fin n
  | ⟨a, h⟩, ⟨b, _⟩ => ⟨((n - b) + a) % n, mlt h⟩
```
This approach is unattractive because it relies on the fact that
`Nat.add` is defined using recursion on the second argument.

The impact on this repo was small, but we want to evaluate the impact
on Mathlib.

closes #4413
github-merge-queue bot pushed a commit that referenced this issue Jun 11, 2024
The performance issue at #4413 is due to our `Fin.sub` definition.
```
def sub : Fin n → Fin n → Fin n
  | ⟨a, h⟩, ⟨b, _⟩ => ⟨(a + (n - b)) % n, mlt h⟩
```
Thus, the following runs out of stack space
```
example (a : UInt64) : a - 1 = a :=
  rfl
```
at the `isDefEq` test
```
(a.val.val + 18446744073709551615) % 18446744073709551616 =?= a.val.val
```

From the user's perspective, this timeout is unexpected since they are
using small numerals, and none of the other `Fin` basic operations (such
as `Fin.add` and `Fin.mul`) suffer from this problem.

This PR implements an inelegant solution for the performance issue. It
redefines `Fin.sub` as
```
def sub : Fin n → Fin n → Fin n
  | ⟨a, h⟩, ⟨b, _⟩ => ⟨((n - b) + a) % n, mlt h⟩
```
This approach is unattractive because it relies on the fact that
`Nat.add` is defined using recursion on the second argument.

The impact on this repo was small, but we want to evaluate the impact on
Mathlib.

closes #4413
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants