Skip to content

Conversation

@caleridas
Copy link
Collaborator

After removing theta invariant violation from dead node elimination from llvm/opt, try to make HLS handling similarly regular.

After removing theta invariant violation from dead node elimination from
llvm/opt, try to make HLS handling similarly regular.
@caleridas caleridas requested review from haved and phate January 8, 2025 23:17
@caleridas
Copy link
Collaborator Author

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.

@phate
Copy link
Owner

phate commented Jan 9, 2025

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.

@sjalander
Copy link
Collaborator

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.

@phate
Copy link
Owner

phate commented Jan 13, 2025

So if I see this correctly, it might be that we do not even need this PR:

  1. distribute constant is discussed in DO NOT MERGE: Remove ineffective distribute-constants optimization #734 whether it should be removed
  2. and UnusedStateRemoval might be able to be replaced by InvariantValueRedirection

@github-actions
Copy link

The execution time of dynamatic/pivot has changed
  Golden cycle time: 1190
  Simulated cycles: 1181

@sjalander
Copy link
Collaborator

@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.

@github-actions
Copy link

The execution time of dynamatic/pivot has changed
  Golden cycle time: 1190
  Simulated cycles: 1181

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants