fastmachinelearning / hls4ml

Machine learning on FPGAs using HLS

https://fastmachinelearning.org/hls4ml

Apache License 2.0

1.22k stars 396 forks source link

Fix for cloned stream that is subsequently flattened #708

Closed jmduarte closed 1 year ago

jmduarte commented 1 year ago

Description

Fixes #707. The issue is if a cloned stream is subsequently flattened, the original stream is used later instead of the clone. I think this is because the InplaceVariable is not correctly updated.

Somewhat related, there is also a bug in concatenate1d when you consider the case that a stream is not truly from a 1D array, but from a flattened 2D array. I tried to use @rfforelli's fix for this, but it seems to not be quite working. Maybe he has some input?

Type of change

[x] Bug fix (non-breaking change that fixes an issue)

Tests

Added a test that should fail on main but succeed on this branch

Test Configuration:

Checklist

[x] I have read the guidelines for contributing.
[x] I have commented my code, particularly in hard-to-understand areas.
[x] I have made corresponding changes to the documentation.
[x] My changes generate no new warnings.
[x] I have installed and run pre-commit on the files I edited or added. (to be done later)
[x] I have added tests that prove my fix is effective or that my feature works.

jmitrevs commented 1 year ago

Flatten is one of the operations that often breaks in the current main branch. The qonnx branch has quite a few fixes there, because the inplace variable as is in main is generally broken when working with optimizers. It sets what the inplace variable points to too early, and optimizers often break this. Let me take a look more carefully at this fix.

jmitrevs commented 1 year ago

I actually have the test fail in main, ingest-qonnx-master, and clone_flatten branches, but I think the form of the code in myproject.cpp produced by both the ingest-qonnx-master and clone_flatten branches seems correct. I would recommend using the ingest-qonnx-master branch for these cases, because as I mentioned, inplace variables are broken in main.

jmduarte commented 1 year ago

OK, the new test should now succeed in clone_flatten but fail in main.

The test fails in main for two reasons: (1) the InplaceVariable problem and (2) the incorrect concatenate1d HLS code for the case when the input stream depth is not the same as full size of the array (e.g. if they are from flattened 2D streams).

This PR has a band-aid solution for (1) and a good solution for (2).

Perhaps, the best way to proceed is

Check that if we add the solution to (2) to the ingest-qonnx-master, the test passes.
If so, then we update this PR to just solve (2) since (1) will be solved by the more general ingest-qonnx-master fixes.
We can also add the new test to the ingest-qonnx-master branch (after it's rebased to include changes from this branch).

jmitrevs commented 1 year ago

If I copy nnet_merge_stream.h from clone_flatten to ingest-qonnx-master, the test passes there. So (2) is definitely a bug to fix. I think I'll try to extract the inplace variable stuff from from ingest-qonnx-master and make it a separate PR, in order to make the merging of that PR easier.

One thing, can you check if the same fix is needed for quartus? On a first glance it looks to me like it's needed there, too. The code is at https://github.com/fastmachinelearning/hls4ml/blob/main/hls4ml/templates/quartus/firmware/nnet_utils/nnet_merge_stream.h#L139-L165.

jmduarte commented 1 year ago

@jmitrevs OK, I confirm that the added test (including Quartus) passes if I merge this branch with #714. So should we plan to merge them both at the same time?

vloncar commented 1 year ago

What's the deal with the failed test?

jmitrevs commented 1 year ago

I assume the failed tests are ones that succeed when #714 is also merged.