Skip to content
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

Quartus Backend #245

Closed
wants to merge 46 commits into from
Closed

Conversation

hamzajaved780
Copy link
Contributor

@hamzajaved780 hamzajaved780 commented Nov 19, 2020

This PR integrates Quartus backend with the hls4ml frontend (PR #195). Backend can selected by changing the backend config variable to Quartus / Vivado. Several functions have been pushed back to their backend-specific template and writer files. This allows easier integration of new backends by just defining new template and writer files. This solution still isn't perfect and a further restructuring of the front-end is needed for future seamless integrations.

In addition, the following layers are currently supported on the Quartus backend:

  1. Dense Latency / Large with binary/ternary support.
  2. Activations (The same as in Vivado)

There is also partial support for sparse dense layers, however there are some scaling issues with wide networks.

@jmitrevs
Copy link
Contributor

jmitrevs commented Dec 1, 2020

Just a minor question: are the .DS_store files added on purpose?

@thesps
Copy link
Contributor

thesps commented Dec 1, 2020

In addition to the .DS_store files (which I don't think should be there), @hamzajaved780 could you remove the converted test models and associated files? They shouldn't be included.

@hamzajaved780
Copy link
Contributor Author

hamzajaved780 commented Dec 8, 2020

Was a TF 2.3 related issue, should be fixed now!

@jmitrevs
Copy link
Contributor

jmitrevs commented Dec 8, 2020

Was a TF 2.3 related issue, should be fixed now!

It seems to be fixed.

@jmitrevs
Copy link
Contributor

jmitrevs commented Dec 8, 2020

I think the following change is needed:

diff --git a/hls4ml/writer/vivado_writer.py b/hls4ml/writer/vivado_writer.py
index aa8dc1c..8b68649 100644
--- a/hls4ml/writer/vivado_writer.py
+++ b/hls4ml/writer/vivado_writer.py
@@ -498,7 +498,7 @@ class VivadoWriter(Writer):
             line = line.replace('myproject',model.config.get_project_name())
 
             if 'set_part {xcku115-flvb2104-2-i}' in line:
-                line = 'set_part {{{}}}\n'.format(model.config.get_config_value('XilinxPart'))
+                line = 'set_part {{{}}}\n'.format(model.config.get_config_value('FPGAPart'))
             elif 'create_clock -period 5 -name default' in line:
                 line = 'create_clock -period {} -name default\n'.format(model.config.get_config_value('ClockPeriod'))

@@ -516,7 +516,7 @@ class VivadoWriter(Writer):
         for line in f.readlines():
             line = line.replace('myproject', model.config.get_project_name())
             if '-part' in line:
-                line = 'synth_design -top {} -part {}\n'.format(model.config.get_project_name(), model.config.get_config_value('XilinxPart'))
+                line = 'synth_design -top {} -part {}\n'.format(model.config.get_project_name(), model.config.get_config_value('FPGAPart'))
 
             fout.write(line)
         f.close()

scripts/hls4ml Outdated
@@ -1,11 +1,12 @@
#!/usr/bin/env python
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this needs to continue using env, not a hardcoded python path.

Copy link
Contributor

Choose a reason for hiding this comment

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

Fixed in current version.

def _find_reports(self, sln_dir, top_func_name, full_report=False):
csim_file = sln_dir + '/csim/report/{}_csim.log'.format(top_func_name)
if os.path.isfile(csim_file):
_show_csim_report(csim_file)
Copy link
Contributor

Choose a reason for hiding this comment

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

should be self._show...


syn_file = sln_dir + '/syn/report/{}_csynth.rpt'.format(top_func_name)
if os.path.isfile(syn_file):
_show_synth_report(syn_file, full_report)
Copy link
Contributor

Choose a reason for hiding this comment

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

should be self._show...

@thesps thesps mentioned this pull request Dec 16, 2020
@jmitrevs
Copy link
Contributor

Some of the front end problems that I have mentioned and related issues have been fixed in my branch:

https://github.com/jmitrevs/hls4ml/tree/quartus

@jmitrevs
Copy link
Contributor

I noticed that the quartus nnet_utils are inside the firmware directory while the Vivado ones are parallel to the firmware directory. Would it make sense to use the same location?

I suppose given the different pragmas it doesn't make sense to try to unify these, though I wonder if there's too much code duplication and the potential to deviate for unnecessary reasons. Is there something better we can do?

@vloncar
Copy link
Contributor

vloncar commented Dec 29, 2020

I noticed that the quartus nnet_utils are inside the firmware directory while the Vivado ones are parallel to the firmware directory. Would it make sense to use the same location?

At the same level, yes.

I suppose given the different pragmas it doesn't make sense to try to unify these, though I wonder if there's too much code duplication and the potential to deviate for unnecessary reasons. Is there something better we can do?

It's not just the pragmas, the algorithms themselves are different, just look at Dense layer and activations. I don't see nnet_utils being shared, but we can and will reuse as much of template/writer (i.e., python) code as possible.

@jmitrevs
Copy link
Contributor

jmitrevs commented Oct 6, 2021

Closing the old quartus pull request, which has been superseded.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants