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

with statements should have weak references to combinational groups #1421

Open
rachitnigam opened this issue Apr 27, 2023 · 4 comments
Open
Labels
C: Internal Changes to compiler internals S: Available Can be worked upon

Comments

@rachitnigam
Copy link
Contributor

I ran into a bug where even though a combinational group had been detached from a component (using drain on component.comb_groups), it was retained in memory by an invoke-with statement. Probably the right thing here would be for with statements to carry weak references to combinational groups and have the ownership of the combinational group associated with only the component's field.

@rachitnigam rachitnigam added C: Calyx Extension or change to the Calyx IL S: Available Can be worked upon labels Apr 27, 2023
@sampsyo
Copy link
Contributor

sampsyo commented Apr 27, 2023

Sounds good. I am contractually obligated to also mention that flattening (#1183 et al.) would address this problem too by tying all AST nodes' lifetimes to a single lifetime for the whole program.

@rachitnigam
Copy link
Contributor Author

Good point! However, in this case, I want to turn things into weak references to explicitly model the fact that they are invalid. I don't think that even in a flat representation, you truly want everything to have the lifetime of a program or even a component.

Building from your blog post, one trick to play here is using the upper bit of the index for a flat representation to denote whether something is valid or not and using that to perform this kind of invalidation

@sampsyo
Copy link
Contributor

sampsyo commented Apr 28, 2023

Ah yes, you are very wise—integers-as-pointers wouldn't itself solve the problem of "notifying" the invoke AST node that the comb group it refers to is gone. Good idea to play tricks with the reference integer itself to denote validity.

@rachitnigam rachitnigam added C: Internal Changes to compiler internals and removed C: Calyx Extension or change to the Calyx IL labels May 8, 2023
@rachitnigam
Copy link
Contributor Author

Hm, this change would also imply that we should carry WRCs for Group and StaticGroup in Enable and StaticEnable which would be a pretty dramatic change. However, I think the overall idea of clarifying the ownership policy for these structs---that only the component owns then and once removed from it, they are no longer valid---seems sane to me.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C: Internal Changes to compiler internals S: Available Can be worked upon
Projects
None yet
Development

No branches or pull requests

2 participants