-
Notifications
You must be signed in to change notification settings - Fork 15
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
Simplify HLS loop dead node elimination #726
base: master
Are you sure you want to change the base?
Conversation
After removing theta invariant violation from dead node elimination from llvm/opt, try to make HLS handling similarly regular.
This looks like the right thing to do, but I am not entirely certain of the intent of why hls dead node elimination is divergent. |
The UnusedStateRemoval transformation is currently nowhere used except in its own tests. The original transformation is in remove-unused-state.hpp/*.cpp, but this one had some problems which stepped on my foot while refactoring (it is/was buggy). So I cleaned it up, implemented some tests along with it to highlight the bugs (while preserving the buggy semantics), and replaced the usage of the old one with the new one. After that, the new and refactored version was used. At some point, @sjalander came and needed to merge something back again from David and the newly used one was replaced again with the old messy one. Now, the newly refactored one is lying around (and diverged again from the old one after the back-merge), while the old one is still used. In other words, it is a mess, and we went often one step forward with the HLS part and then two steps backward again. Most of the problems with the HLS part originate from the fact that it was not merged back into master regularly. We would need some effort to clean it up, and stop having two diverging versions around. @haved For visibility. |
The UnusedStateRemoval/remove-unused-state pass operates on an RVSDG/LLVM graph, i.e., there shouldn't be any HLS specific nodes in the graph. Therefore, existing passes should be able to replace this code, e.g., InvariantValueRedirection might be able to do the same thing. @phate @caleridas @haved NTNU's cluster is being updated today and tomorrow, so I currently don't have a setup where I can try to make changes. |
So if I see this correctly, it might be that we do not even need this PR:
|
|
@caleridas @phate should we merge this PR? PR #833 took the UnusedStateRemoval code into use, making this PR more relevant. The distribute_constants is still part of the HLS flow as it is about optimizing the final timing of the synthesized accelerator, making it more challenging to assess potential impact if it is removed. |
|
After removing theta invariant violation from dead node elimination from llvm/opt, try to make HLS handling similarly regular.