Fix Vitis Conv1D/2D latency strategy #815
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
jmitrevs
approved these changes
Jun 19, 2023
8 tasks
calad0i
pushed a commit
to calad0i/hls4ml
that referenced
this pull request
Jul 1, 2023
Fix Vitis Conv1D/2D latency strategy
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
This fixes #805. Vitis HLS compiler has problems with synthesizing pointer expressions like
*(res++). Using array indexing resolves the compilation issue, but then the design fails to meet timing, due to the inlining of theconv_1d_latency_cl(ditto 2d) function. This seems to be on a per-layer (or model) basis, since it works sometimes. It's best to disable it to meet timing in the general case, but then people can explore enabling it to save a few cycles. I left a comment along those lines. I implemented it only for Vitis, but if people encounter the issue in Vivado we can use the same fix there, again bringing the backends to single-source HLS implementation.Also, I used the opportunity to sneak a commit that fixes the Vitis pragma warnings regarding
DATA_PACKpragma. This seems to be a leftover from a bad merge. The workaround used brings it in line with the rest of the codebase where we know it works.Type of change
Tests
Only synthesis will confirm this is the fix, and we don't (yet) have synthesis tests for Vitis, so this one is trust me bro type of PR. But @Duchstf will properly test it and report back, right Duc?
Checklist
I did all the things in the checklist.