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

Added push/pop API #290

Draft
wants to merge 2 commits into
base: main
Choose a base branch
from
Draft

Conversation

dewert99
Copy link
Contributor

@dewert99 dewert99 commented Jan 2, 2024

Fixes #188

I think I was able to get a decent version of push/pop working. I added a few tests for it but it may still be buggy, so we should probably add some more before merging this.

I added an associate type UndoLog to the Analysis trait to allow users to opt-in to the extra cost tracking this information. Ideally, it would default to () which restores the old behaviour, but unfortunately, associated type defaults are not stable so this is a breaking change. This idea could also be used for explanations so calling an explanation-related method when explanations are disabled would be a compile-time error rather than a run-time error.

Calling pop currently requires explanations to be enabled since it uses id_to_node, the UndoLog could store its own copy of this information but this would be redundant if explanations and push/pop were both enabled. Another idea would be to store each uncanonical id's original node directly in the egraph, this would allow us to avoid storing nodes in EClass.parents, EGraph.pending, and EGraph.analysis_pending, eliminate ExplainNode.node, and make the Egraph::id_to_X methods not require explanations to be enabled. Would it be worth doing a separate PR for this first?

Calling pop also currently requires that the analysis doesn't do anything, as I am still trying to figure out the best way to revert an arbitrary merge implementation.

Calling push requires pending/analysis_pending to be empty, users can deal with this by calling rebuild before push but technically calling process_unions is sufficient. I've been wondering if it is worth making process_unions public since rebuild_classes is only necessary for Searchers. This would also allow pop to be optimized for the case where rebuild_classes hasn't been called since the last push.

@dewert99 dewert99 mentioned this pull request Feb 13, 2024
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.

1 participant