-
Notifications
You must be signed in to change notification settings - Fork 51
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
Generalize sharing #1042
Generalize sharing #1042
Conversation
…s can be transformed
Hey @calebmkim can you update the write up to discuss what code changes you made and how it works? It’s hard to understand complex changes like these without context |
Just added context on writeup. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks like a great start! I have a couple of code questions but I'd also like to get on a call and talk through the implementation before merging things.
Ah, looks like some changes from the dominators stuff did creep in here. You can try fixing this by resetting your local to only have changes from the remote: Next, you can write a commit that remove all the dominators stuff from this PR. |
One other thing I realized: we should ignore any cell that is marked as a |
@rachitnigam edit: I also handled the changed it to ignore ref_cells even though I forgot to say that in the commit message. edit: For some reason it is failing some of the Dahila frontend tests. From what I can see just doing a quick look through, it looks like the errors are coming from the fact that my stuff is doing |
The infer-static-timing pass has some generalized logic to detect if a group is using a component. Maybe that will be helpful? |
Just to expand on that comment a little bit more: the pass looks for the |
I don't know if this makes sense, but do we perhaps need a more flexible notion of what "looks like a write" to a given component? Like, maybe we want to consider that a component can be "used" for either reading or writing, just like a register, and both might involve enabling |
@rachitnigam I have updated the PR. The biggest change I have made is on |
Handles the first part of #1018!! |
Combined the
minimize_regs
pass andresource-sharing
pass into 1 pass. Right now, they're both being done on theminimize_regs
pass.More detail:
So for
minimize_regs
I added 3 fields:shareable, state_shareable
andcont_cells
. I implemented theConstructVisitor
trait forMinimizeRegs
, and the code I used to do that is almost the same as whenConstructVisitor
was implemented inResourceSharing
. It takes in a context and looks for primitives and components marked with "share" and "state_share" attribute and adds it to the appropriate field.We used the cont_cells field so that we could have a way of completely filtering out any cells used in any continuous assignments.
In the
initialize
method ofMinimizeRegs
we clone the "share" and "state_share" fields and pass them as arguments when we create the new LiveRangeAnalysis (line 91 ofminimize_regs.rs
) Here is what LiveRangeAnalysis does.Let's look at the
new
function of LiveRangeAnalysis.Its
build_live_ranges
functions basically the same as before. The only reason why we need the "state_share" is because, for a given cell, we want to check its cell type for membership in thestate_share
HashSet (edit: there is a helper methodis_shareable_component
to help do this) instead of checking if its type isstd_reg
. Even though these things are basically the same right now, it may be helpful in the future for components marked with thestate_share
attribute.Then (looking at lines 294-310 of
live_range_analysis.rs
), it goes through each group and combinational group of the component, and looks for any use of a cell that has the "share" attribute. If it does, then it adds it as "live" in the group it is contained in. There is a helper function,add_shareable_ranges
, to help factor out common code when doing this task.Lmk if you need more detail: I can comment directly on the code if that's the case.