-
Notifications
You must be signed in to change notification settings - Fork 421
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
hls4ml Optimization API [Part 2] #809
hls4ml Optimization API [Part 2] #809
Conversation
I will add pre-commit additionally, last time I ran it, some tests were broken, so will add it a subsequent commit. |
This is ready for review, seems that pre-commit can re-arrange the order of includes in C++ header files and it could cause compilation error. |
20ed996
to
0f0adc4
Compare
We merged part 1. Should we merge part 2? |
I'm reviewing it. Slowly 😃 . But it's next in line, then HGQ. |
The pytest error is unrelated to the PR so from my side this can be merged. I'll let Vladimir give the last OK. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why switching the default impl to verilog here? What's the impact of the added optimization on firmware performance and synth time?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There shouldn't be any noticeable impact. There are 2 reasons this change was made:
-
When you use VHDL with Resource strategy the synthesis report gives 0 BRAM (which cannot be the case at all, especially since Verilog shows the correct BRAM). I suspect this is something to do out-of-context synthesis in Vivado but I haven't figured out how to enable / disable it. This is explained more in: Vivado synthesis report - zero BRAM utilisation (OOC) #798
-
It's "functionally" not correct to do co-simulation / validation with Verilog and synthesis with VHDL (since HLS always generates both formats anyway). Yes, if one is correct and the other is wrong then it's an issue with the HLS compiler and we can't do much about it. But my reasoning is that using we should test and "deploy" the same set of end-files.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Then this should probably go into a separate PR I guess, as it changs the behavior of the library while not using dsp aware pruning. How about let's isolate bo3z@e53ec9c into another PR?
hls4ml/writer/vivado_writer.py
Outdated
newline += indent + '#pragma HLS DATAFLOW \n' | ||
|
||
model_cfg = model.config.get_config_value('HLSConfig')['Model'] | ||
if ( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems that if unrolled resource conv layers are used, dataflow will not be used. Also, if n_partition
is not 1, the global II won't the be larger than the internal dense II (reuse_factor
)?
This PR was refactored to introduce the new unrolled implementation as a "strategy", to be an alternative to existing latency and resource strategies. This allowed the the matrix-vector multiplication kernel to be used as a function, simplifying the integration with the rest of the code. The PR also has changes to the top pipeline style pragma, so the config now includes a new "auto" option (the default) which allows the optimizer to choose the best one. All of pipeline style decisions are now made in the new optimizer, instead of being scattered around the HLSConfig class and the backend. One more minor change may come. Since we will have multiple new strategies and optimization options, it was suggested to give this optimization technique a name and move it to a submodule of that name. Discussion on this is welcome. |
All the comments from the last discussion were addressed (renamed the strategy and moved the files to a new submodule). It's now ready to be merged |
Description
Type of change
Tests
Checklist
pre-commit
on the files I edited or added.