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

refactor: getUnfoldableConst*? #5997

Merged
merged 3 commits into from
Jan 19, 2025
Merged

refactor: getUnfoldableConst*? #5997

merged 3 commits into from
Jan 19, 2025

Conversation

Kha
Copy link
Member

@Kha Kha commented Nov 7, 2024

Continuation from #5429: eliminates uses of these two functions that care about something other than reducible defs/theorems, then restricts the function definition to these cases to be more true to its name.

@Kha Kha requested a review from leodemoura November 7, 2024 16:22
@@ -1263,10 +1263,7 @@ private def processAssignment' (mvarApp : Expr) (v : Expr) : MetaM Bool := do

private def isDeltaCandidate? (t : Expr) : MetaM (Option ConstantInfo) := do
match t.getAppFn with
| Expr.const c _ =>
match (← getUnfoldableConst? c) with
| r@(some info) => if info.hasValue then return r else return none
Copy link
Member Author

Choose a reason for hiding this comment

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

hasValue checks unnecessary now that getUnfoldableConst*? is documented to return defs/thms.

Comment on lines 66 to 100
@[inline] private def matchConstAux {α} (e : Expr) (failK : Unit → MetaM α) (k : ConstantInfo → List Level → MetaM α) : MetaM α := do
let .const name lvls := e
| failK ()
let (some cinfo) ← getUnfoldableConst? name
| failK ()
k cinfo lvls
Copy link
Member Author

Choose a reason for hiding this comment

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

I found the code clearer after inlining this helper, its name is too mysterious otherwise

matchConstAux f.getAppFn (fun _ => unfoldProjInstWhenInstances? e) fun fInfo fLvls => do
| .app f _ => do
let .const fName fLvls := f.getAppFn | unfoldProjInstWhenInstances? e
match (← getUnfoldableConst? fName) with
Copy link
Member Author

Choose a reason for hiding this comment

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

This is the only former matchConstAux caller that cares about unfolding

@Kha Kha force-pushed the push-xqpvqmqwlqxp branch from 27c91a5 to a719d61 Compare November 7, 2024 16:25
@Kha
Copy link
Member Author

Kha commented Nov 7, 2024

!bench

@leanprover-bot
Copy link
Collaborator

Here are the benchmark results for commit a719d61.
There were significant changes against commit c779f3a:

  Benchmark           Metric          Change
  =====================================================
+ big_do              branch-misses    -7.7%  (-18.1 σ)
+ big_omega.lean MT   branch-misses    -4.5%  (-22.1 σ)
- lake env            task-clock        6.8%   (12.2 σ)
- lake env            wall-clock        6.8%   (12.8 σ)
+ reduceMatch         instructions     -1.2% (-367.6 σ)

@github-actions github-actions bot added the toolchain-available A toolchain is available for this PR, at leanprover/lean4-pr-releases:pr-release-NNNN label Nov 7, 2024
leanprover-community-mathlib4-bot added a commit to leanprover-community/batteries that referenced this pull request Nov 7, 2024
leanprover-community-mathlib4-bot added a commit to leanprover-community/mathlib4 that referenced this pull request Nov 7, 2024
@leanprover-community-bot leanprover-community-bot added the builds-mathlib CI has verified that Mathlib builds against this PR label Nov 7, 2024
@leanprover-community-bot
Copy link
Collaborator

leanprover-community-bot commented Nov 12, 2024

Mathlib CI status (docs):

  • ❗ Batteries/Mathlib CI will not be attempted unless your PR branches off the nightly-with-mathlib branch. Try git rebase 9f42368e1ae749ae122bfb44e2f082e6cc3ba604 --onto 456e6d2b791bbc560b304b28a60f35db7072c306. (2024-11-12 14:06:45)
  • ❗ Batteries/Mathlib CI will not be attempted unless your PR branches off the nightly-with-mathlib branch. Try git rebase 5fb2e892c8bea5d359c0cd9ee59b271155db5538 --onto d4070d4bfbeb821b1614455e74198f5a11f557d5. (2025-01-18 23:35:01)

@Kha Kha force-pushed the push-xqpvqmqwlqxp branch from 0f8e0ce to 1c45597 Compare January 18, 2025 23:11
@Kha Kha force-pushed the push-xqpvqmqwlqxp branch from 1c45597 to b4a1ca1 Compare January 18, 2025 23:13
@Kha Kha enabled auto-merge January 18, 2025 23:16
@Kha Kha added this pull request to the merge queue Jan 18, 2025
Merged via the queue into leanprover:master with commit 8a8417f Jan 19, 2025
14 checks passed
@Kha Kha deleted the push-xqpvqmqwlqxp branch January 19, 2025 01:16
luisacicolini pushed a commit to opencompl/lean4 that referenced this pull request Jan 21, 2025
Continuation from leanprover#5429: eliminates uses of these two functions that
care about something other than reducible defs/theorems, then restricts
the function definition to these cases to be more true to its name.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
builds-mathlib CI has verified that Mathlib builds against this PR toolchain-available A toolchain is available for this PR, at leanprover/lean4-pr-releases:pr-release-NNNN
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants