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

lattice: Don't forget to pass down lattice in Conditional tmerge #47992

Merged
merged 1 commit into from
Dec 25, 2022

Conversation

Keno
Copy link
Member

@Keno Keno commented Dec 25, 2022

Also add some extra cases that before were taken care of by the tmerge_fast_path at the entry to the tmerge code. I briefly considered splitting an extra slow path function to avoid the one === in the ConstsLattice, but in the non-equality case that check should be quite fast, so it didn't seem worth it.

@Keno Keno requested a review from aviatesk December 25, 2022 04:18
Copy link
Member

@aviatesk aviatesk left a comment

Choose a reason for hiding this comment

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

@nanosoldier runbenchmarks("inference", vs=":master")

@Keno
Copy link
Member Author

Keno commented Dec 25, 2022

@nanosoldier runbenchmarks("inference", vs=":master")

@nanosoldier
Copy link
Collaborator

Your benchmark job has completed - possible performance regressions were detected. A full report can be found here.

Also add some extra cases that before were taken care of by the
tmerge_fast_path at the entry to the tmerge code. I briefly
considered splitting an extra slow path function to avoid the
one `===` in the ConstsLattice, but in the non-equality case
that check should be quite fast, so it didn't seem worth it.
@Keno
Copy link
Member Author

Keno commented Dec 25, 2022

Added an appropriate fast path, which should hopefully fix the perf regression:

@nanosoldier runbenchmarks("inference", vs=":master")

@nanosoldier
Copy link
Collaborator

Your benchmark job has completed - no performance regressions were detected. A full report can be found here.

@Keno Keno merged commit 162ee48 into master Dec 25, 2022
@Keno Keno deleted the kf/latticeconddown branch December 25, 2022 12:36
vtjnash added a commit that referenced this pull request Aug 16, 2023
In 162ee48, the added code causes us to violate the tmerge_fast_path
requirements on the lattice. This was causing the fall-though from the
earlier tmerge_fast_path to not return correct answers to inference
anymore. Adding back another tmerge_fast_path, on the types, allows us
to recover correctness without regressing accuracy to before #47992.

Also added a test case for an example in which tmerge_fast_path does not
return a correctly limited answer, since it does not model UnionAll
complexity growth.
vtjnash added a commit that referenced this pull request Aug 21, 2023
In 162ee48, the added code causes us to violate the tmerge_fast_path
requirements on the lattice. This was causing the fall-though from the
earlier tmerge_fast_path to not return correct answers to inference
anymore. Adding back another tmerge_fast_path, on the types, allows us
to recover correctness without regressing accuracy to before #47992.

Also added a test case for an example in which tmerge_fast_path does not
return a correctly limited answer, since it does not model UnionAll
complexity growth.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants