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

Vivado accelerator axi master interface #683

Open
wants to merge 14 commits into
base: main
Choose a base branch
from

Conversation

GiuseppeDiGuglielmo
Copy link
Contributor

Description

🐥 👉 Creating a new PR to allow edits from maintainers. Previous PR. 👈 🐥

Extend the VivadoAccelerator backend to support AXI-m data movers for inputs and outputs. An ideal setup is a Xilinx Zynq/ZynqMP board (ARM core + programmable logic).

The backend generates the Python drivers for Pynq-Z2.

Tests

You can test it with this example on Pynq-Z2.

@jmduarte jmduarte force-pushed the vivado-accelerator-axi-master-interface branch from 17c0fa5 to 109773c Compare November 18, 2022 18:27
@jmduarte jmduarte self-requested a review November 18, 2022 18:28
@jmduarte jmduarte added enhancement please test Trigger testing by creating local PR branch labels Nov 18, 2022
@jmduarte jmduarte requested a review from thesps November 18, 2022 18:30
@jmduarte jmduarte added please test Trigger testing by creating local PR branch and removed please test Trigger testing by creating local PR branch labels Nov 30, 2022
@jmduarte jmduarte added please test Trigger testing by creating local PR branch and removed please test Trigger testing by creating local PR branch labels Nov 30, 2022
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.

Some first-round comments

@@ -124,6 +131,23 @@ def write_axi_wrapper(self, model):
newline += indent + '#pragma HLS INTERFACE ap_ctrl_none port=return\n'
if model.config.get_config_value("IOType") == 'io_stream':
newline += indent + '#pragma HLS DATAFLOW\n'
elif '//hls-fpga-machine-learning insert load weights' in line:
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 strongly discourage this style of code in the writer. What's the point of templates if they are blank and all the logic is written by the writer? Just clutters the writer. This is much better addressed with a template that contains compile-time constants to select the functionality and just fill out the few lines with specific weights

Copy link
Contributor

Choose a reason for hiding this comment

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

Isn't this a copy mainly from vivado_writer.py lines 130-139? Do they both need to change?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@jmitrevs, yes I moved the code from the vivado_write.py to the vivado_accelerator_write.py.

@@ -139,10 +163,12 @@ def write_axi_wrapper(self, model):
newline += indent + '}\n'
elif io_type == 'io_stream':
newline = ''
newline += 'LOAD_INPUT_OUTER_LOOP:\n'
Copy link
Contributor

Choose a reason for hiding this comment

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

What's the point of this?

Copy link
Contributor

Choose a reason for hiding this comment

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

Also, can we remove the explicit creation of loops and move that to the template, as suggested above?

@@ -188,6 +216,35 @@ def write_axi_wrapper(self, model):
f.close()
fout.close()

def modify_project_cpp(self, model):
Copy link
Contributor

Choose a reason for hiding this comment

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

Lol. Just add a flag to not write the interface in the VivadoWriter instead of this approach.

@@ -37,10 +37,10 @@ def print_array_to_cpp(self, var, odir, write_txt_file=True):

if write_txt_file:
h_file.write("#ifndef __SYNTHESIS__\n")
h_file.write(var.definition_cpp() + ";\n")
h_file.write("static " + var.definition_cpp() + ";\n")
Copy link
Contributor

Choose a reason for hiding this comment

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

  1. Why at all?
  2. Why like this and not in definition_cpp()?


def predict(self, X, debug=False, profile=False, encode=None, decode=None):
"""
Obtain the predictions of the NN implemented in the FPGA.
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we convert this docstring to Google style, like is used elsewhere in the code?

@jmduarte jmduarte added this to the v0.8.0 milestone Feb 24, 2023
@jmduarte jmduarte added please test Trigger testing by creating local PR branch and removed please test Trigger testing by creating local PR branch labels Mar 19, 2023
@jmduarte jmduarte modified the milestones: v0.8.0, v1.0.0 Oct 20, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement please test Trigger testing by creating local PR branch
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants