-
Notifications
You must be signed in to change notification settings - Fork 53
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
Redesign ControlID
to provide a better Interface
#1366
Comments
Thank you for the super awesome writeup @calebmkim!
This is something I've been pondering as well–we use the controlID stuff in slightly, but importantly, different ways in various passes like TDCC (which, AFAIK, doesn't even use ControlID). I think a good question is what would be a good representation/set of node ID that works for every pass?
We can even do one step further and change Once a pass is done with it's CFG-based computation, we can use a The big thing to note about this design point is that it doesn't let you add anything to the CFG, just query it and modify the groups underneath it. However, it is still powerful for analyses. Maybe we can cleverly design a
Yes! In fact, the current unsharing pass does something similar in computing a CFG but does so in a hacky way. @EclecticGriffin might have more insight on how exactly it works.
I think we can use groups in place of basic blocks. Also, it is not necessary for us to build a CDFG for the kinds of passes we perform today but maybe useful to think about in the future. |
Yes indeed; thanks for the really useful writeup, @calebmkim! I think you did a good job mapping out what it would take to approach either of these two ideas. Perhaps it goes without saying, but this discussion also implicates the idea of "flattening" the entire IR so that integer IDs are already available (they are the way you identify control nodes), as in #1183 and friends. I think it would be good to direct the discussion toward deciding what we will do: one of these, both, or some kind of intermediate point? Maybe a way to make that decision would be to resolve this question from @rachitnigam:
Could we do an audit of the way in which these IDs are use so we can confidently describe what all these passes want from a set of IDs? |
Overview
Currently, the
ControlID pass
provides methods that can label the control stmts with u64's, and we can query for the label of a given control stmt. We want to improve this analysis to provide a better interface for the user.There are two main ideas discussed in #1358. The first is a more minor change to improve the interface of ControlID, the second is a more major change to get
ControlID
to actually build a full CDFG.Improve ControlID Interface to Give Tokens instead of u64's
Par(u64)
, see: New Type Idiom) instead of u64. This way, we can build an interface in which the users of the ControlID Analysis never actually look at the u64's inside the tokens.ParThread(u64)
to signify the control stmt is a direct child of a par block, but other times, we might want the same control statement to be wrapped as aSeq(u64)
(or whatever that control statement actually is).ParThread(u64)
token for a control statement that isn't actually the direct child/thread of a par block, then we will probably want to produce an errorControlID should produce a CFG
ControlID
should take in a control block, and internally build a full CFG. Then, users should just be able to make queries about the CFG, whichControlID
should also support by internally analyzing the CFG that it has built.of instructions that you know they are just going to always just execute sequentially). We could try to support something like this for Calyx, organizing the graph into blocks of groups that we know are always just going to execute sequentially. That seems a bit different than the current sketch, where single groups/invokes themselves seem to be the "basic blocks" of the CFG. 2) @rachit also mentioned SSA form. Is this something that we should try to implement for Calyx? It seems like we would have to aggresively instantiate new cells if we were to do this.
The text was updated successfully, but these errors were encountered: