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

[prim] Remove primgen and replace with virtual cores #23555

Draft
wants to merge 31 commits into
base: master
Choose a base branch
from

Conversation

a-will
Copy link
Contributor

@a-will a-will commented Jun 7, 2024

Add back prim_pkg enum for legacy support, and mention that it is deprecated. Some IPs reference the enums directly.

@a-will a-will force-pushed the fusesoc-compat branch 2 times, most recently from 65eb0ec to 19af483 Compare June 7, 2024 07:04
@marnovandermaas marnovandermaas added the Type:Enhancement Feature requests, enhancements label Jun 7, 2024
@a-will a-will force-pushed the fusesoc-compat branch 2 times, most recently from 458c9e4 to 4009fe6 Compare June 9, 2024 23:45
@a-will
Copy link
Contributor Author

a-will commented Jun 9, 2024

Interesting, it looks like mixing with a newer fusesoc might require at least Python 3.9 to work with the other dependencies.

ERROR: Ignored the following versions that require a different python version: 1.0.0 Requires-Python >=3.9,<4.0; 1.1.0 Requires-Python >=3.9
ERROR: Could not find a version that satisfies the requirement jsonschema2md==1.1.0 (from versions: 0.1.0, 0.1.1, 0.2.0.post1, 0.2.1, 0.3.0, 0.4.0, 0.9.0)
ERROR: No matching distribution found for jsonschema2md==1.1.0

@a-will a-will force-pushed the fusesoc-compat branch 2 times, most recently from e7dd88c to b2b9aab Compare June 10, 2024 06:02
@a-will
Copy link
Contributor Author

a-will commented Jun 10, 2024

Newer fusesoc also has a different directory layout compared to the OT fork. This was the OT fork's layout:

  • {--build-root}/
    • sim-vcs/
      • fusesoc-created metadata, Makefile, and file list
    • src/
      • Exported source files

But newer fusesoc has this directory layout:

  • {--work-root}/
    • Contains fusesoc-created metadata, Makefile, and file list
    • src/
      • Exported source files

Accommodating the new tree needed fixes to various paths, including CFLAGS definitions in hjson files. In the current example, {work-root} is set to dvsim's {build_dir}/fusesoc-work.

In addition, for ralgen, the position field was needed to be set to prepend for generated cores, else the files would come after the *_env_pkg.sv that would depend on them.

Now, some simulations work, but there is still a gap...

@a-will a-will force-pushed the fusesoc-compat branch 4 times, most recently from 787aca8 to 5344016 Compare June 17, 2024 04:34
@a-will
Copy link
Contributor Author

a-will commented Oct 31, 2024

This PR is way out of date now, but since @olofk has released fusesoc 2.4, I think we don't need my random development tag anymore. 2.4 should be able to handle virtual cores as needed.

@olofk
Copy link
Contributor

olofk commented Oct 31, 2024

@a-will I think that depends on the OT requirements. If you are fine with pulling in cores from one vendor during a build, then virtual cores should do the trick.

However, as I understand it, OT wants to be able to select implementation at compile-time, and in that case you still need something like primgen.

With that said, I believe FuseSoC 2.4 should have all the functionality needed to implement what you need. I have done some experimenting implementing the equivalent to primgen as a FuseSoC filter instead of a generator and it looks promising.

Happy to discuss this further.

@a-will
Copy link
Contributor Author

a-will commented Oct 31, 2024

@a-will I think that depends on the OT requirements. If you are fine with pulling in cores from one vendor during a build, then virtual cores should do the trick.

However, as I understand it, OT wants to be able to select implementation at compile-time, and in that case you still need something like primgen.

With that said, I believe FuseSoC 2.4 should have all the functionality needed to implement what you need. I have done some experimenting implementing the equivalent to primgen as a FuseSoC filter instead of a generator and it looks promising.

Happy to discuss this further.

It's unclear to me if OT actually needs that, though. All of our in-tree top-level fusesoc cores end up binding a specific prim library, and an out-of-tree integrator would almost certainly do the same for their actual chip. The build recipes for synthesis often can't be reused across technologies, and across integrations for the same opentitan IP, the top-level generally uses different RTL, so we end up with independent top-level cores anyway. We'd end up with different YAML for each real target, so sharing that core file across implementations doesn't seem to provide a benefit.

There may be a bit of a rub with gate-level simulation applications, though, especially if you just want to redo the existing block-level simulations with your own prim library. I'd guess that the full integration level still uses its own core file for GLS, but is doing block-level simulation with a different prim library a supported activity? I'm not sure.

We'd probably achieve the original setup's capabilities if only we could provide fusesoc (as a parameter to its invocation) which implementations of virtual cores to include for the specified top-level core file. For example, it could be arguments to provide additional VLNVs to add to the build (using their default target). Then the top-level core file wouldn't need to explicitly pull them in.

@a-will
Copy link
Contributor Author

a-will commented Nov 1, 2024

For example, it could be arguments to provide additional VLNVs to add to the build (using their default target). Then the top-level core file wouldn't need to explicitly pull them in.

@olofk If that bit above is interesting, the specifying of additional VLNVs would be akin to how hierarchical synthesis flows work. The top-level (and other sub-cores) may have dangling references, but the missing netlists would get specified as additional sources, then linked in during the build. For fusesoc, this would mean writing in a "depends" arrow from the top-level VLNV to the additional VLNVs, and the solver should handle them as though they were written in the core file Edit: Actually, this might be just additional cores thrown into the Requirement, with no need to modify the dependency graph.

@HU90m HU90m force-pushed the fusesoc-compat branch 2 times, most recently from 980f9e1 to 90f83e5 Compare November 25, 2024 18:19
@a-will
Copy link
Contributor Author

a-will commented Nov 26, 2024

Looks like fusesoc 2.4 isn't being used in the sim runs in CI, and something broke with the CI file changes. I guess the API is a bit different for github actions. Maybe s/parameters/inputs?

Copy link
Contributor

@marnovandermaas marnovandermaas left a comment

Choose a reason for hiding this comment

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

Amazing PR! Left a few comments from looking at the code.

Comment on lines 18 to 19
# TODO: prim_pkg_legacy is deprecated
- lowrisc:prim:prim_pkg_legacy
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe good to put this in a separate commit, just to separate it from the prepend changes.

@@ -73,6 +73,7 @@ generate:
parameters:
name: otbn
ip_hjson: ../../../data/otbn.hjson
position: prepend
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this go in the previous commit?

Comment on lines 28 to 29
# TODO: prim_pkg is deprecated
- lowrisc:prim:prim_pkg
Copy link
Contributor

Choose a reason for hiding this comment

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

Will we create an issue to track this?

Comment on lines 24 to 25
# TODO: prim_pkg_legacy is deprecated
- lowrisc:prim:prim_pkg_legacy
Copy link
Contributor

Choose a reason for hiding this comment

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

This change is similar to a change made in an earlier commit.

Copy link
Contributor

Choose a reason for hiding this comment

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

Are these CDC waivers tested in CI? If not, have we tested this ourselves?

@@ -14,6 +14,8 @@ filesets:
- "fileset_partner ? (partner:systems:scan_role_pkg)"
- "!fileset_partner ? (lowrisc:systems:ast)"
- "!fileset_partner ? (lowrisc:systems:scan_role_pkg)"
- "!fileset_partner ? (lowrisc:prim:prim_pkg_legacy)"
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this have a todo that this will be removed in the future?

Copy link
Contributor

Choose a reason for hiding this comment

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

Also true for other core files in top_earlgrey

@@ -12,6 +12,8 @@ filesets:
- lowrisc:systems:ast
- lowrisc:systems:topgen
- lowrisc:systems:padring
- lowrisc:prim:prim_pkg_legacy
Copy link
Contributor

Choose a reason for hiding this comment

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

Add todo?

@olofk
Copy link
Contributor

olofk commented Nov 26, 2024

Regarding paths, I haven't checked exactly what the problem is, but I would recommend using --work-root instead of --build-root since that gives you more deterministic behaviour. You might also want to use --system-name to get a fixed name for the build artifacts.

@a-will
Copy link
Contributor Author

a-will commented Nov 26, 2024

Regarding paths, I haven't checked exactly what the problem is, but I would recommend using --work-root instead of --build-root since that gives you more deterministic behaviour.

That is what I did in this PR: https://github.com/lowRISC/opentitan/pull/23555/files#diff-8ed9c1256f9282d31ab3a7f150e2e95eaae62a546151c7203117aaa70451129e

HU90m and others added 12 commits January 30, 2025 19:33
Switched ascent lint over to use FuseSoC's `--work-root` as oppose to
`--build-root`.

Signed-off-by: Hugo McNally <[email protected]>
Make many lint targets depend on the generic prims.

Co-authored-by: Hugo McNally <[email protected]>
Signed-off-by: Alexander Williams <[email protected]>
They were all missing prim_subreg_pkg in their dependencies.

Signed-off-by: Alexander Williams <[email protected]>
The macros depend on ipgen outputs, so avoid pulling them in with the
ordinary prims.

Signed-off-by: Alexander Williams <[email protected]>
Move the non-templated otp_ctrl_pkg out of the ipgen folder.
Remove unnecessary virtual VLNVs exported by otp_ctrl ipgen outputs.
Add direct dependencies to ipgen outputs, including pwrmgr_pkg from the
same top.

Signed-off-by: Alexander Williams <[email protected]>
@blueluna
Copy link

I have been working on a similar feature as olofk/fusesoc#727 for fusesoc under the guidance of @olofk. This feature adds a lock file to fusesoc, which guides the core selection during the fusesoc run. Guiding in selecting core version and virtual core implementations. I opened a draft pull request, olofk/fusesoc#728, for collaboration. We should collaborate on a guided virtual core selection feature.

@HU90m
Copy link
Member

HU90m commented Jan 31, 2025

Lock files are a brilliant addition to FuseSoC that I'm very pleased to see. However, I don't think it solves the same problem as olofk/fusesoc#727 intends to.

Currently we have to add the generic primitives explicitly to a huge number files (e.g. hw/ip/csrng/csrng.core):

  files_lint:
    depend:
      - lowrisc:prim_generic:all

# ...

    filesets_append:
      - files_lint

@a-will 's solution is to allow VLNVs to be added in the CLI instead:

fusesoc --add-vlnv lowrisc:prim_generic:all lint lowrisc:ip:csrng

Another solution to this problem that would keep the cli cleaner would be to allow users to set default implementations for a given virtual core in the fusesoc.conf.

@olofk
Copy link
Contributor

olofk commented Jan 31, 2025

Another solution to this problem that would keep the cli cleaner would be to allow users to set default implementations for a given virtual core in the fusesoc.conf.

Yes! This was perhaps not expressed clearly enough, but the lock files have two purposes. The first is pinning versions, and the other one is exactly what you describe above. Locking down virtuals to a concrete implementation. We felt it was so close in scope with the first purpose of the lock files so we decided to put it in the same file rather than in fusesoc.conf

This is very much developed with OpenTitan in mind, so let us know your thoughts. What we perhaps should consider adding is to allow expressing the virtual->concrete mapping on the command-line similar to what @a-will did with the --add-vlnv feature.

@Razer6
Copy link
Member

Razer6 commented Jan 31, 2025

This is very much developed with OpenTitan in mind, so let us know your thoughts. What we perhaps should consider adding is to allow expressing the virtual->concrete mapping on the command-line similar to what @a-will did with the --add-vlnv feature.

This is exactly what, we as Rivos and also other integrators need. There is the public prim_generic library. But integrators will have their own, e.g., a prim_rivos. We want to run the block level tests for example on our prim library. Pinning the virtual core provider of an IP specifically to for example lowrisc:prim_generic:all also creates a conflict. That is done on a higher level of the hierarchy.

@olofk
Copy link
Contributor

olofk commented Jan 31, 2025

I suggest moving the discussion to olofk/fusesoc#728 so that we can work out what is best for everyone involved.

Experiment with a fusesoc that includes the --add-vlnv arg.

Signed-off-by: Alexander Williams <[email protected]>
Ensure all IPs use the generic prims by default when simulating.

Signed-off-by: Alexander Williams <[email protected]>
… levels"

This reverts commit 986c249.

Signed-off-by: Alexander Williams <[email protected]>
Add the choice of prims to the fusesoc call instead.

Signed-off-by: Alexander Williams <[email protected]>
Use the --add-vlnv option to resolve fusesoc virtual core dependencies.

Signed-off-by: Alexander Williams <[email protected]>
@a-will
Copy link
Contributor Author

a-will commented Feb 1, 2025

I've changed this PR to use a development release that includes the --add-vlnv option to show how we'd like that to work. I don't have complete working knowledge of dvsim, so the configs may be more verbose than necessary right now. (And only some of them are done, of course!)

Changing to this method dropped the changed files down by ~120 files.

@a-will
Copy link
Contributor Author

a-will commented Feb 1, 2025

TODOs (WIP):

  • Uniquify chip_ral_pkg VLNV. There are conflicts between different tops.
  • Move the bazel symlinks to a subdirectory that contains a FUSESOC_IGNORE file. We get warnings for duplicate cores, but they're the exact same files! ...or move the fusesoc --cores-root to not be the root of the repo ([fusesoc] Move all cores under the hw directory #26142)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Type:Enhancement Feature requests, enhancements
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants