-
Notifications
You must be signed in to change notification settings - Fork 653
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
Add example accelerator using HLS #2056
Conversation
the Great Common Denominator (GCD) of two integers. This tutorial | ||
builds on the sections :ref:`mmio-accelerators` and | ||
:ref:`incorporating-verilog-blocks`. The code for this example can | ||
be found in ``/generators/hls-example`` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would use gh-file-ref
to have it auto-generate a link:
This :gh-file-ref:`scripts/firesim-setup.sh` script initializes additional submodules and then invokes FireSim's ``build-setup.sh`` script with the ``--library`` to properly initialize FireSim as a library submodule in Chipyard. |
Adding an HLS project | ||
--------------------------------------- | ||
|
||
In this tutorial, we use Vitis HLS, version 2023.2. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The CI is using an older version of the tool FYI.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also maybe I would point to some docs/link to Vitis HLS. Isn't there restrictions on where this can be used (and if so, should we mention something about that here)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I looked closer on the product site and the documentation but I couldn't find any restrictions on where Vitis HLS can be used. Possibly it's in the license somewhere, but I don't really want to crawl through that... Some forum comments implied that Vitis HLS has been used in commercial tapeouts, so if there are rules they don't seem to be very strict. We should be able to just leave it up to the user to know what they're doing.
.. code-block:: none | ||
vitis_hls run_hls.tcl |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All code block directives need an empty space before the main text of the block.
In our case, we include a ``Makefile`` to script running HLS. To generate the | ||
verilog files using the Makefile, run: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Below you mention this also does the "move files to vsrc" step. So maybe I would add some extra saying "to script running HLS (as well as copying files around as explained later)"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we change the naming to something like CanHavePeripheryHLSGCD
. Ditto for the config. fragment name.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you also mention the TODO of how to connect up to an AXI interface.
generators/hls-example/run_hls.tcl
Outdated
@@ -0,0 +1,8 @@ | |||
open_project -reset proj_gcd_example | |||
add_files accel/HLSAccel.cpp |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
HPP is missing?
generators/hls-example/run_hls.tcl
Outdated
add_files accel/HLSAccel.cpp | ||
set_top HLSAccelBlackBox | ||
open_solution -reset "solution1" | ||
set_part {xcvu9p-flgb2104-2-i} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do you need a part number? This is FPGA specific so maybe the docs should mention this RTL that is made is specific to an FPGA (with part number listed here) but can be run in RTL sim.
generators/hls-example/run_hls.tcl
Outdated
set_top HLSAccelBlackBox | ||
open_solution -reset "solution1" | ||
set_part {xcvu9p-flgb2104-2-i} | ||
create_clock -period 10 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see that both the FPGA part number and the clock period should probably configurable from the makefile and piped to the Chisel.
Super super cool! Thanks Ella! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, I think it might be good to combine this into the existing GCD example area instead of separate SBT project s.t. people can either create a GCD with 1. Chisel 2. SV 3. SV from HLS
build.sbt
Outdated
@@ -263,6 +263,11 @@ lazy val rocc_acc_utils = (project in file("generators/rocc-acc-utils")) | |||
.settings(libraryDependencies ++= rocketLibDeps.value) | |||
.settings(commonSettings) | |||
|
|||
lazy val hls_accel = (project in file("generators/hls-example")) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this should go in generators/chipyard/example, instead of its own project
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Small nits otherwise LGTM
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM pending CI
Related PRs / Issues:
Type of change:
Impact: