fastmachinelearning / hls4ml

Machine learning on FPGAs using HLS
https://fastmachinelearning.org/hls4ml
Apache License 2.0
1.19k stars 390 forks source link

Decouple pipeline style from strategy #781

Closed vloncar closed 1 year ago

vloncar commented 1 year ago

Description

We control the top function pipeline style with the strategy, and while this works, sometimes the change to the top pipelining is forced by a layer (the conv layer) and as a consequence this causes all other layers to use resource strategy which is unintended. Decoupling the strategy from the pipeline style fixes this. The explicitly set resource strategy still forces the dataflow style to be used. The configuration can be explicitly set with PipelineStyle (values can be pipeline and dataflow), but normally the users would still continue using the Strategy config parameter. Fixes #699 and #759.

Type of change

Tests

I didn't add any extra tests. The best way to test this is to verify the generated HLS has the correct pragmas and parameters.h. I manually tested a whole lot of combinations of this. Checking the IR in python may also be possible, but I'm not sure if this will capture all combinations.

Checklist

jmitrevs commented 1 year ago

Do we understand why the tests failed? Otherwise, the requested change is quite minor.

vloncar commented 1 year ago

Not sure why the tests failed, but certainly it is not due to the changes in the code. Since this started happening recently and we didn't change the CI configuration, it may be due to the increasing number of parallel tests each creating a new environment. Not sure why it doesn't deterministically fail. We need to investigate further. You can rerun the tests or run them locally to be sure that indeed nothing got broken. I ran locally, it all passed.

jmitrevs commented 1 year ago

Unless people object, I will merge this tomorrow. But is this for 0.8 or 0.7.1?

vloncar commented 1 year ago

How about we merge #780, fix the capitalization issue observed in #782, release that as 0.7.1 and then start merging for 0.8.0 with this PR?