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

Use vendored-in primitives from OpenTitan #894

Merged
merged 4 commits into from
May 27, 2020
Merged

Conversation

imphil
Copy link
Contributor

@imphil imphil commented May 25, 2020

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 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.

@imphil
Copy link
Contributor Author

imphil commented May 25, 2020

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.

@imphil imphil force-pushed the no-ram2p branch 11 times, most recently from ae81b1d to 3f3424c Compare May 25, 2020 22:31
@imphil
Copy link
Contributor Author

imphil commented May 25, 2020

In terms of testing we should be good as well.

  • CI tests are green, which includes
    • Ibex compliance
    • Lint & Co.
  • ibex_core UVM tests pass
  • The simple system example can execute code.
  • The Arty A7-35 example synthesizes (I don't have a board to test)
  • icache UVM tests pass

@imphil imphil requested review from tomeroberts and rswarbrick May 25, 2020 22:34
Comment on lines -7 to -13
*
* 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.
Copy link
Contributor

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.

Copy link
Contributor Author

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
Copy link
Contributor

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?

Copy link
Contributor Author

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])

@imphil imphil force-pushed the no-ram2p branch 2 times, most recently from 6b26f4a to 13317b5 Compare May 26, 2020 18:51
@imphil
Copy link
Contributor Author

imphil commented May 26, 2020

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).

@vogelpi vogelpi self-requested a review May 27, 2020 07:16
Copy link
Contributor

@vogelpi vogelpi left a 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!

Copy link
Contributor

@rswarbrick rswarbrick 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, 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...".

imphil added 4 commits May 27, 2020 10:16
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.
@imphil
Copy link
Contributor Author

imphil commented May 27, 2020

Good catch @rswarbrick, fixed.

@imphil imphil merged commit 8b42024 into lowRISC:master May 27, 2020
@imphil imphil deleted the no-ram2p branch May 27, 2020 09:23
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.

3 participants