Skip to content

Stage initial set of changes for the Catapult backend #956

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

Merged
merged 63 commits into from
Apr 15, 2024

Conversation

dgburnette
Copy link
Contributor

@dgburnette dgburnette commented Jan 12, 2024

A# Description
This is the initial merge of a Catapult backend for HLS4ML. This merge includes all of the python changes and the top-level template files but not the nnet_utils/* files at this point in time.

📝 Please include a summary of the change.

  • Please also include relevant motivation and context.
  • List any dependencies that are required for this change.

Type of change

For a new feature or function, please create an issue first to discuss it
with us before submitting a pull request.

Note: Please delete options that are not relevant.

  • Bug fix (non-breaking change that fixes an issue)
  • Documentation update
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • A new research paper code implementation
  • Other (Specify)

Tests

📝 Please describe the tests that you ran to verify your changes.

  • Provide instructions so we can reproduce.
  • Please also list any relevant details for your test configuration.

Test Configuration:

Checklist

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

@@ -560,6 +560,7 @@ def initialize(self):
if self.model.config.is_resource_strategy(self) and self.model.config.backend.name in [
'Vivado',
'VivadoAccelerator',
'Catapult',
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Truthfully, I think this needs to go away, also for Vivado. The standard representation should not have model-specific hooks. These changes should be made in the backend.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I made a general issue about this: #957

@@ -233,6 +233,11 @@ def definition_cpp(self, name_suffix='', as_reference=False):
type=self.type.name, name=self.name, suffix=name_suffix, shape=self.size_cpp(), pragma=self.pragma
)

class CatapultArrayVariableDefinition(VariableDefinition):
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would prefer all these additions to not be placed here but in backends/catatapult/catapult_types.py. (The quartus and vivado additions should also be moved to corresponding files.)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(Moving the quartus and vivado additions is outside the scope of this PR, though; this PR is just for catapult.)

@jmitrevs jmitrevs added the please test Trigger testing by creating local PR branch label Jan 13, 2024
@jmitrevs jmitrevs added please test Trigger testing by creating local PR branch and removed please test Trigger testing by creating local PR branch labels Jan 15, 2024
@@ -0,0 +1,104 @@
import json
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should remove if not used.

vhdl 1
verilog 1
export 0
vsynth 0
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Might want to rename this to not be Vivado synthesize (vsynth)

@jmitrevs
Copy link
Contributor

Based on our meeting, we have the following action items:

  1. In backends/catapult/passes, remove any passes that are not used.
  2. Also in backends/catapult/passes, note passes that are used but identical with what is used for Vivado. We will figure out how we want to handle those.
  3. Remove any build scripts or parts of build scripts that are not used. Remove references to Vivado.
  4. Put ac_types and other submodules in hls4ml/template/cataplult. (I think you can just do git submodule add in that directory)
  5. Add migrated nnet_utils to the template/catapult/nnet_utils. This includes some common utils and layer-specific utils. Do not include untested layers.
  6. All the layers that are included in nnet_utils should have corresponding tests in test/pytest. Generally this could be done by just enabling the catapult backend on already existing tests instead of writing new tests.

@jmitrevs jmitrevs added please test Trigger testing by creating local PR branch and removed please test Trigger testing by creating local PR branch labels Feb 22, 2024
@dgburnette dgburnette added please test Trigger testing by creating local PR branch and removed please test Trigger testing by creating local PR branch labels Apr 10, 2024
@@ -1,7 +1,7 @@
#ifndef NNET_INSTR_GEN_H_
#define NNET_INSTR_GEN_H_

#include "nnet_helpers.h"
#include "nnet_utils/nnet_helpers.h"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This change causes some pytests to fail.

@dgburnette dgburnette added please test Trigger testing by creating local PR branch and removed please test Trigger testing by creating local PR branch labels Apr 10, 2024
@jmitrevs jmitrevs added please test Trigger testing by creating local PR branch and removed please test Trigger testing by creating local PR branch labels Apr 11, 2024
@jmitrevs jmitrevs added please test Trigger testing by creating local PR branch and removed please test Trigger testing by creating local PR branch labels Apr 11, 2024
@jmitrevs jmitrevs requested a review from vloncar April 12, 2024 22:10
@vloncar vloncar added please test Trigger testing by creating local PR branch and removed please test Trigger testing by creating local PR branch labels Apr 15, 2024
Copy link
Contributor

@vloncar vloncar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good. Thanks @dgburnette and your team for all the hard work in putting this together.

@vloncar vloncar merged commit 2531afe into fastmachinelearning:main Apr 15, 2024
calad0i added a commit to calad0i/hls4ml that referenced this pull request Apr 16, 2024
jmitrevs added a commit that referenced this pull request Apr 16, 2024
fix unwanted tested file change in #956
steltze pushed a commit to steltze/hls4ml that referenced this pull request Jun 20, 2024
…rning#956)

* Stage initial set of changes for the Catapult backend

* applied some changes for issues reported by pre-commit. But pre-commit still reorders backends/__init__.py incorrectly

* final changes for clean pre-commit

* minor edits

* Checkin

* Add file

* pre-commit format

* add in nnet_utils files

* format changes for pre-commit

* run flows by netlist type

* update design pragmas on some blocks. cleaned up TCL script

* move AC submodules under hls4ml/templates/catapult

* merged in latest changes from mainline

* remove bad submodules

* recreate AC submodules in hls4ml/templates/catapult

* pre-commit fixes

* pre-commit fixes

* turn on Catapult backend testing

* removed io_parallel testing for Catapult backend

* add Catapult

* added Catapult

* added Catapult

* added Catapult to some pytests

* Added concept of ProjectDir to distinguish the project directory of the HLS tool from the ProjectName which is used for the cpp file and top function name

* better handling of c++ testbench data files. enhanced directory naming.

* fix syntax

* workaround from Giuseppe

* Add concept of ProjectDir for Catapult which is different from ProjectName that gets used for the top function name and the cpp files

* add new file from Giuseppe

* improvements to project management, reporting and testbench

* include new file in generation of parameters.h

* add hard_tanh for io_parallel. formatting

* Full path to the header nnet_helpers.h is necessary in the include (check if this is not an issue with recent versions of Catapult)

* Avoid ceiling function from the math library: ceil(n/d) ---> (n+d-1)/n

* These are mostly workarounds for the BUP synyhesis of a testing model (should these changes make in something more general?)

* revert format back to what clang-format currently enforces

* simplification from Giuesspe

* Fixes for bottom-up handling of libraries

* pre-commit format fixes

* fix loops

* consolidate prj scripts

* cleanup pragmas

* switch from using ssh to https for submodules

* fix include path for non-catapult install

* update pytest environment

* CL 1100381

* CL 1098112

* roll in latest changes. turn off Catapult variants of test_binary_cnn and test_cnn_mnist_qkeras for now

* fix test failure

* disable Catapult test for pytorch until it is supported

* disable Catapult for pytorch tests

* Simpler submodule initialization for CI

---------

Co-authored-by: David Burnette <[email protected]>
Co-authored-by: Giuseppe Di Guglielmo <[email protected]>
Co-authored-by: Jovan Mitrevski <[email protected]>
Co-authored-by: Vladimir Loncar <[email protected]>
steltze pushed a commit to steltze/hls4ml that referenced this pull request Jun 20, 2024
steltze pushed a commit to steltze/hls4ml that referenced this pull request Jun 20, 2024
…rning#956)

* Stage initial set of changes for the Catapult backend

* applied some changes for issues reported by pre-commit. But pre-commit still reorders backends/__init__.py incorrectly

* final changes for clean pre-commit

* minor edits

* Checkin

* Add file

* pre-commit format

* add in nnet_utils files

* format changes for pre-commit

* run flows by netlist type

* update design pragmas on some blocks. cleaned up TCL script

* move AC submodules under hls4ml/templates/catapult

* merged in latest changes from mainline

* remove bad submodules

* recreate AC submodules in hls4ml/templates/catapult

* pre-commit fixes

* pre-commit fixes

* turn on Catapult backend testing

* removed io_parallel testing for Catapult backend

* add Catapult

* added Catapult

* added Catapult

* added Catapult to some pytests

* Added concept of ProjectDir to distinguish the project directory of the HLS tool from the ProjectName which is used for the cpp file and top function name

* better handling of c++ testbench data files. enhanced directory naming.

* fix syntax

* workaround from Giuseppe

* Add concept of ProjectDir for Catapult which is different from ProjectName that gets used for the top function name and the cpp files

* add new file from Giuseppe

* improvements to project management, reporting and testbench

* include new file in generation of parameters.h

* add hard_tanh for io_parallel. formatting

* Full path to the header nnet_helpers.h is necessary in the include (check if this is not an issue with recent versions of Catapult)

* Avoid ceiling function from the math library: ceil(n/d) ---> (n+d-1)/n

* These are mostly workarounds for the BUP synyhesis of a testing model (should these changes make in something more general?)

* revert format back to what clang-format currently enforces

* simplification from Giuesspe

* Fixes for bottom-up handling of libraries

* pre-commit format fixes

* fix loops

* consolidate prj scripts

* cleanup pragmas

* switch from using ssh to https for submodules

* fix include path for non-catapult install

* update pytest environment

* CL 1100381

* CL 1098112

* roll in latest changes. turn off Catapult variants of test_binary_cnn and test_cnn_mnist_qkeras for now

* fix test failure

* disable Catapult test for pytorch until it is supported

* disable Catapult for pytorch tests

* Simpler submodule initialization for CI

---------

Co-authored-by: David Burnette <[email protected]>
Co-authored-by: Giuseppe Di Guglielmo <[email protected]>
Co-authored-by: Jovan Mitrevski <[email protected]>
Co-authored-by: Vladimir Loncar <[email protected]>
steltze pushed a commit to steltze/hls4ml that referenced this pull request Jul 11, 2024
…rning#956)

* Stage initial set of changes for the Catapult backend

* applied some changes for issues reported by pre-commit. But pre-commit still reorders backends/__init__.py incorrectly

* final changes for clean pre-commit

* minor edits

* Checkin

* Add file

* pre-commit format

* add in nnet_utils files

* format changes for pre-commit

* run flows by netlist type

* update design pragmas on some blocks. cleaned up TCL script

* move AC submodules under hls4ml/templates/catapult

* merged in latest changes from mainline

* remove bad submodules

* recreate AC submodules in hls4ml/templates/catapult

* pre-commit fixes

* pre-commit fixes

* turn on Catapult backend testing

* removed io_parallel testing for Catapult backend

* add Catapult

* added Catapult

* added Catapult

* added Catapult to some pytests

* Added concept of ProjectDir to distinguish the project directory of the HLS tool from the ProjectName which is used for the cpp file and top function name

* better handling of c++ testbench data files. enhanced directory naming.

* fix syntax

* workaround from Giuseppe

* Add concept of ProjectDir for Catapult which is different from ProjectName that gets used for the top function name and the cpp files

* add new file from Giuseppe

* improvements to project management, reporting and testbench

* include new file in generation of parameters.h

* add hard_tanh for io_parallel. formatting

* Full path to the header nnet_helpers.h is necessary in the include (check if this is not an issue with recent versions of Catapult)

* Avoid ceiling function from the math library: ceil(n/d) ---> (n+d-1)/n

* These are mostly workarounds for the BUP synyhesis of a testing model (should these changes make in something more general?)

* revert format back to what clang-format currently enforces

* simplification from Giuesspe

* Fixes for bottom-up handling of libraries

* pre-commit format fixes

* fix loops

* consolidate prj scripts

* cleanup pragmas

* switch from using ssh to https for submodules

* fix include path for non-catapult install

* update pytest environment

* CL 1100381

* CL 1098112

* roll in latest changes. turn off Catapult variants of test_binary_cnn and test_cnn_mnist_qkeras for now

* fix test failure

* disable Catapult test for pytorch until it is supported

* disable Catapult for pytorch tests

* Simpler submodule initialization for CI

---------

Co-authored-by: David Burnette <[email protected]>
Co-authored-by: Giuseppe Di Guglielmo <[email protected]>
Co-authored-by: Jovan Mitrevski <[email protected]>
Co-authored-by: Vladimir Loncar <[email protected]>
steltze pushed a commit to steltze/hls4ml that referenced this pull request Jul 11, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
please test Trigger testing by creating local PR branch
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants