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

[IR] done condition should not be an assignment #864

Open
rachitnigam opened this issue Jan 11, 2022 · 3 comments
Open

[IR] done condition should not be an assignment #864

rachitnigam opened this issue Jan 11, 2022 · 3 comments
Labels
C: Internal Changes to compiler internals S: Available Can be worked upon

Comments

@rachitnigam
Copy link
Contributor

Currently, ir::Port can either be a port on a cell or a group's hole. This was a decision I made when reimplementing the IR and was mostly based on the idea that walking through the assignments should give you access to everything that "looks like" an assignment, including writes to a group's done condition. This was a bad idea.

done condition

This is pretty uncontroversially a bad idea–most of the assignment handling code that needs to update ports ignores holes and vice-versa. It also leads to the detached-mutability pattern that shows up across the code base where you have detach all the assignments from a group before mutating them because an assignment may a write to a done hole and cause an immutable borrow of the group leading to an error.

Additionally, there should be exactly one write to the done signal. A bunch of passes and backends implicitly make this assumption. Finally, done signals are very different from other ports and need to be handled differently. This should be reflected in the IR.

We can make the write to the done hole a separate field on the group data structure. This is the same decision that @cgyurgyik made for the CIRCT implementation of Calyx.

Holes in Read Positions

There is a broader problem with groups showing up in read positions because this make it necessary for them to be ir::Port. Ideally, they should be a separate thing altogether but I don't yet have a good idea about how to go about doing this.

@sampsyo
Copy link
Contributor

sampsyo commented Jan 11, 2022

Rad; this sounds a pretty legit change. In more than one way, it kinda reminds me of terminators in LLVM basic blocks… the decision there, however, is that the terminator is "just another instruction," but well-formed programs must have exactly one and they must be last. (And there is a method to get this unique instruction.)

Perhaps it's needless to say, but one could imagine providing a utility to iterate over all the expressions in a group, including the "normal" assignments and the done condition, for cases where that's necessary.

@cgyurgyik
Copy link
Collaborator

The Terminator trait is used for the CIRCT Calyx GroupDonePort as well.

@rachitnigam rachitnigam added C: Calyx Extension or change to the Calyx IL S: Discussion needed Issues blocked on discussion labels Jan 12, 2022
@rachitnigam rachitnigam added S: Available Can be worked upon and removed S: Discussion needed Issues blocked on discussion labels Feb 9, 2022
@rachitnigam
Copy link
Contributor Author

The first step to doing this would be to expose a function on groups to return the assignment representing the done condition.

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

Successfully merging a pull request may close this issue.

3 participants