-
Notifications
You must be signed in to change notification settings - Fork 585
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
Use vendored-in primitives from OpenTitan #894
Conversation
Here we go, the final battle to bring (more) order into mess of copied files between OT and Ibex. Please have a look at the individual commits for slightly more details. There should be no functional change after this PR is applied. |
ae81b1d
to
3f3424c
Compare
In terms of testing we should be good as well.
|
* | ||
* The two ports are in Read-First Mode: when reading from and writing to the same address | ||
* simultaneously, the old content is returned, before the new content is written. New content | ||
* is made available to both ports with one cycle delay. | ||
* | ||
* Simultaneous write operations by both ports to the same address are to be avoided: The data | ||
* written to memory is not determined. |
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 usually find it useful to have this information somewhere but agree that now that this ram_2p.sv
is just a wrapper for the actual prim, it is no longer the right place.
I think we should add this information in the actual primitive upstream.
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 removed it because we cannot make those guarantees in the wrapper, and the non-wrapper doesn't actually have any such guarantees at the moment. (lowRISC/opentitan#2267)
@@ -7,3 +7,6 @@ git+https://github.com/lowRISC/edalize.git@ot | |||
|
|||
# Development version with OT-specific changes | |||
git+https://github.com/lowRISC/fusesoc.git@ot | |||
|
|||
pyyaml | |||
mako |
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.
Is this for primgen
? Does that now get run as part of an Ibex build?
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.
Yes. (pyyaml is also used for ibex_config.py but hasn't been included in the requirements file so far and was installed on CI runners by "luck" [not real luck, it's a dependency of fusesoc and edalize])
6b26f4a
to
13317b5
Compare
I've updated this PR to use the new vendor.py script, took the updated check_requirements.py, and found a small change which wasn't related to this PR (#901). |
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.
This looks good to me. Thanks @imphil for the massive PR!
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.
Looks good, with a couple of minor nits.
I think the sudo
used for removing python3-yaml in azure-pipelines.yml
is on the wrong commit (gets added by "Install wheel...", but should be in the previous patch).
If you're re-pushing anyway, there's also a typo in the title of "Move Verlator simutil...".
Pyyaml is needed for primgen (coming next), and for ibex_config.py. Install it through python-requirements.txt. This requires, unfortunately, an uninstallation of the distribution-provided version first (otherwise pip cannot install it).
... and to get around warnings when using pip without wheel being present. No functional change expected.
lowRISC/opentitan#2311 added the Verilator memutils to OpenTitan as upstream. This commit is the second part of the story, removing the code from the Ibex repository, and vendoring it back in from OpenTitan. This also superseded lowRISC#844, which has now been included through OpenTitan.
Instead of using copies of primitives from OpenTitan, vendor the files in directly from OpenTitan, and use them. Benefits: - Less potential for diverging code between OpenTitan and Ibex, causing problems when importing Ibex into OT. - Use of the abstract primitives instead of the generic ones. The abstract primitives are replaced during synthesis time with target-dependent implementations. For simulation, nothing changes. For synthesis for a given target technology (e.g. a specific ASIC or FPGA technology), the primitives system can be instructed to choose optimized versions (if available). This is most relevant for the icache, which hard-coded the generic SRAM primitive before. This primitive is always implemented as registers. By using the abstract primitive (prim_ram_1p) instead, the RAMs can be replaced with memory-compiler-generated ones if necessary. There are no real draw-backs, but a couple points to be aware of: - Our ram_1p and ram_2p implementations are kept as wrapper around the primitives, since their interface deviates slightly from the one in prim_ram*. This also includes a rather unfortunate naming confusion around rvalid, which means "read data valid" in the OpenTitan advanced RAM primitives (prim_ram_1p_adv for example), but means "ack" in PULP-derived IP and in our bus implementation. - The core_ibex UVM DV doesn't use FuseSoC to generate its file list, but uses a hard-coded list in `ibex_files.f` instead. Since the dynamic primitives system requires the use of FuseSoC we need to provide a stop-gap until this file is removed. Issue lowRISC#893 tracks progress on that. - Dynamic primitives depend no a not-yet-merged feature of FuseSoC (olofk/fusesoc#391). We depend on the same functionality in OpenTitan and have instructed users to use a patched branch of FuseSoC for a long time through `python-requirements.txt`, so no action is needed for users which are either successfully interacting with the OpenTitan source code, or have followed our instructions. All other users will see a reasonably descriptive error message during a FuseSoC run. - This commit is massive, but there are no good ways to split it into bisectable, yet small, chunks. I'm sorry. Reviewers can safely ignore all code in `vendor/lowrisc_ip`, it's an import from OpenTitan. - The check_tool_requirements tooling isn't easily vendor-able from OpenTitan at the moment. I've filed lowRISC/opentitan#2309 to get that sorted. - The LFSR primitive doesn't have a own core file, forcing us to include the catch-all `lowrisc:prim:all` core. I've filed lowRISC/opentitan#2310 to get that sorted.
Good catch @rswarbrick, fixed. |
Instead of using copies of primitives from OpenTitan, vendor the files
in directly from OpenTitan, and use them.
Benefits:
Less potential for diverging code between OpenTitan and Ibex, causing
problems when importing Ibex into OT.
Use of the abstract primitives instead of the generic ones. The
abstract primitives are replaced during synthesis time with
target-dependent implementations. For simulation, nothing changes. For
synthesis for a given target technology (e.g. a specific ASIC or FPGA
technology), the primitives system can be instructed to choose
optimized versions (if available).
This is most relevant for the icache, which hard-coded the generic
SRAM primitive before. This primitive is always implemented as
registers. By using the abstract primitive (prim_ram_1p) instead, the
RAMs can be replaced with memory-compiler-generated ones if necessary.
There are no real draw-backs, but a couple points to be aware of:
Our ram_1p and ram_2p implementations are kept as wrapper around the
primitives, since their interface deviates slightly from the one in
prim_ram*. This also includes a rather unfortunate naming confusion
around rvalid, which means "read data valid" in the OpenTitan advanced
RAM primitives (prim_ram_1p_adv for example), but means "ack" in
PULP-derived IP and in our bus implementation.
The core_ibex UVM DV doesn't use FuseSoC to generate its file list,
but uses a hard-coded list in
ibex_files.f
instead. Since thedynamic primitives system requires the use of FuseSoC we need to
provide a stop-gap until this file is removed. Issue DV: Use FuseSoC instead of hardcoding file list in ibex_dv.f #893 tracks
progress on that.
Dynamic primitives depend no a not-yet-merged feature of FuseSoC
(Resolve dependencies for generator-created cores olofk/fusesoc#391). We depend on the same
functionality in OpenTitan and have instructed users to use a patched
branch of FuseSoC for a long time through
python-requirements.txt
,so no action is needed for users which are either successfully
interacting with the OpenTitan source code, or have followed our
instructions. All other users will see a reasonably descriptive error
message during a FuseSoC run.
This commit is massive, but there are no good ways to split it into
bisectable, yet small, chunks. I'm sorry. Reviewers can safely ignore
all code in
vendor/lowrisc_ip
, it's an import from OpenTitan.The check_tool_requirements tooling isn't easily vendor-able from
OpenTitan at the moment. I've filed
[util] Move check_tool_requirements into its own directory opentitan#2309 to get that sorted.
The LFSR primitive doesn't have a own core file, forcing us to include
the catch-all
lowrisc:prim:all
core. I've filed[prim_lfsr] Create a separate core file opentitan#2310 to get that sorted.