circt
circt copied to clipboard
llvm
[StandardToHandshake] Replace `insertMergeOps`/liveness analysis with maxSSA
std-to-handshake technically contains it's own attempt at maxSSA through its liveness analysis/merge op insertion (https://github.com/llvm/circt/blob/main/lib/Conversion/StandardToHandshake/StandardToHandshake.cpp#L576).
However, issues remain in the implementation, resulting in some circuits being processed incorrectly (todo: add test case - as far as I remember, this is loop related).
The solution to that was/is to put the CF-level IR into maxSSA form before conversion. For dataflow circuits, maxSSA form brings beauty in form of making all dataflow explicit - any value referenced within a block is defined within the block. The intention is then to make maxSSA a forced precondition of the pass, and remove addMergeOps from std-to-handshake.
The best way forward here would be to get maxSSA into upstream MLIR and leverage that from CIRCT*. 2nd best option would be to get maxSSA into CIRCT as part of the std-to-handshake pass, just as with your BB rewriting. 3rd best (worst) option is to keep it in circt-hls and expect users to magically know that they should have applied a random pass from a random repository that probably doesn't even build anymore, to ensure correct behavior...
*: I have an old (stale) PR to do this. Feel free to pick up the work on that - there's some additional work required to make it more general + make it up-to-date with current MLIR.
Where did you develop the stale PR? Do you have a branch on a github repository somewhere?
Didn't maintain a fork for this, so the best bet is probably to just grab the diff from the PR, apply it onto head-of-LLVM and expect that most things are now out of sync 😛.
I'm taking a shot at this. For the sake of simplicity I went with the second option to include it within CIRCT (I'm also unsure of how to make it general enough so that it has more chance of getting into upstream MLIR).
Do you think it makes sense to have maxSSA as a generic module-level pass (e.g., like the memref flattening pass) outside of std-to-handshake and then have the conversion pass internally call utilities defined by maxSSA? I've made a first attempt at this here. The MaximizeSSAPass converts all of a module's functions into maximal SSA form and at the same time defines a couple utilities (all called circt::maximizeSSA) to convert a value/operation/block/region into maximal SSA form.
Do you think it makes sense to have maxSSA as a generic module-level pass
I think it makes sense, but i also know that there historically has been pushback from having these high-level transformations which doesn't really have anything to do with hardware, live in CIRCT (hence the push for upstreaming it). It then just happens to be that upstream is reluctant to accept changes which apply in the general case (which i can sympathize with, it's not good if a lot of 'unfinished' passes gets upstream). I think if you wanted to pick up the stale PR, there'd be people upstream who's helpful enough to review and suggest feedback.
@Dinistro may provide some insight here wrt. him getting the CFG transformations into CIRCT.
@Dinistro may provide some insight here wrt. him getting the CFG transformations into CIRCT.
As long as you move it into the standard to handshake specific directory (like the merge block insertion), it should be fine.
In general, I agree with Morten that this should ideally be upstreamed. Unfortunately, this is not that easy as there are special cases that need to be treated differently. Thus, it's much harder to implement a clean design that can be reused.
Thank you both for your inputs. For the sake of simplicity I decided to try to include it within CIRCT first (in the StandardToHandshake directory as @Dinistro suggested) and might try at a later time to upstream this into MLIR. PR will follow.