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

Sharing recovery fix #22

Closed

Conversation

sseefried
Copy link
Contributor

The NodeCounts data structure has changed substantially. It is no longer a list of
node/count pairs with the invariant that all child nodes appear after parent nodes. It is
now a list of "dependency groups" each dependency group is a directed acyclic graph (DAG)
that captures the dependencies that an AccSharing node can have on VarSharing nodes.

The function 'filterCompleted' now checks and filters out nodes on a per-dependency group
basis.

This commit also introduces a new trace function in D.A.A.Debug and adds tests for sharing
recovery.

There are also some new dependencies added to accelerate.cabal because we are using
unordered containers.

The NodeCounts data structure has changed substantially. It is no longer a list of
node/count pairs with the invariant that all child nodes appear after parent nodes.  It is
now a list of "dependency groups" each dependency group is a directed acyclic graph (DAG)
that captures the dependencies that an AccSharing node can have on VarSharing nodes.

The function 'filterCompleted' now checks and filters out nodes on a per-dependency group
basis.

This commit also introduces a new trace function in D.A.A.Debug and adds tests for sharing
recovery.

There are also some new dependencies added to accelerate.cabal because we are using
unordered containers.
@sseefried
Copy link
Contributor Author

I realise you've done a lot of changes. This would actually be best pulled into master after commit c16f19d.

I'm loathe to combine this patch with your partially complete recovery of scalar expressions.

@sseefried sseefried closed this May 13, 2011
@sseefried sseefried reopened this May 13, 2011
@sseefried
Copy link
Contributor Author

Just got your last comment. Yes, I realise this conflicts with your last chunk of code. I've mainly sent it through just so you can validate it. If it passes muster then I will integrate with your stuff, or you can do the same.

mchakravarty added a commit that referenced this pull request Mar 18, 2012
* Fixes a problem in sharing recovery that @sseefried (Sean Seefried) highlighted in Issue #22
* Also adds additional tracing in pure code
mchakravarty added a commit that referenced this pull request Mar 18, 2012
These tests are from @sseefried (Sean Seefried)'s pull request for Issue #22: <https://github.com/mchakravarty/accelerate/pull/22>.
@mchakravarty
Copy link
Member

Thanks again for the fix and examples that highlighted an important problem in the original implementation. I actually managed to find a significantly simpler fix. Instead of tracking entire dependency groups, it is sufficient to track the height of shared trees, and then, to ensure that the entries in NodeCounts are ordered by height (higher ones first). When scanning for completed subtrees (to generate let bindings), only pick entries from the front of the list.

This ensures a well-formed result as subtrees are necessarily less high than the parent trees that contain them.

@mchakravarty
Copy link
Member

(This fix is currently only on my "sharing" branch.)

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.

None yet

2 participants