-
Notifications
You must be signed in to change notification settings - Fork 715
fix: avoid unnecessary branching in match compilation #10763
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
Conversation
This reverts commit 0204c5f.
This PR improves match compilation: Branch on variables in the order suggested by the first remaining alternative, and do not branch when the first remaining alternative does not require it. This fixes #10749. (For now this is an experiment to get familiar with the code and the whole problem domain. It is likely overly naive.)
|
Reference manual CI status:
|
|
Mathlib CI status (docs):
|
…lean4 into joachim/issue10749
…achim/issue10749
|
!bench |
|
Benchmark results for 41f4465 against efbbb0b are in! @nomeata |
|
Here are the benchmark results for commit d16a7bf. Benchmark Metric Change
=======================================================================
+ Init.Data.List.Sublist re-elab -j4 maxrss -1.1% (-61.7 σ)
+ channel.lean unbounded_seq -4.0%
+ const_fold instructions -8.1% (-112.4 σ)
+ omega_stress.lean async branch-misses -11.9% (-47.3 σ)
+ omega_stress.lean async branches -12.4% (-1142.0 σ)
+ omega_stress.lean async instructions -12.9% (-1001.0 σ)
+ omega_stress.lean async task-clock -13.0% (-27.7 σ)
+ omega_stress.lean async wall-clock -13.7% (-43.6 σ)
- stdlib type checking 2.0% (22.2 σ) |
|
That’s neat: + const_fold instructions -8.1% (-112.4 σ)
+ omega_stress.lean async instructions -12.9% (-1001.0 σ) |
|
!radar |
|
Benchmark results for c993f9b against efbbb0b are in! @nomeata |
This PR extracts some refactorings from #10763, including dropping dead code and not failing in `inaccessibleAsCtor`, which leadas to (slightly) better error messages, and also on the grounds that the failing alternative may actually be unreachable.
…chim/processInaccessibleAsCtor
…achim/issue10749
|
I’m inclined to merge this, with test suite and mathlib passing and performance gains to be gained. I expect that there are tricky matches that may break with this, but probably for any of these one can construct one that works only with this change. There is a backwards option in place. |
|
!radar |
|
Benchmark results for 2dc3d5a against 705084d are in! @nomeata Major changes (3)
Minor changes (1)
|
This PR improves match compilation: Branch on variables in the order
suggested by the first remaining alternative, and do not branch when the
first remaining alternative does not require it. This fixes #10749. With
set_option backwards.match.rowMajor falsethe old behavior can be turned on.(For now this is an experiment to get familiar with the code and the whole
problem domain. It is likely overly naive.)