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

[virtual-vlnv] Add CLI option to add virtual providers #727

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

a-will
Copy link
Contributor

@a-will a-will commented Jan 30, 2025

Add --add-vlnv to enable adding user-requested VLNVs that are not under the "system" or top core's explicit dependency tree. The intention of this argument is to allow users to specify cores that provide the implementations of virtual VLNVs in these "side" trees. The CLI option allows users to provide these without requiring someone to write a core file with the explicit deps, so targets may be reused across multiple different configurations, and users don't need to have duplicate core files that only differ by virtual providers.

@a-will a-will force-pushed the add-virt-providers branch from 6f75aed to f9182be Compare January 30, 2025 21:48
Add --add-vlnv to enable adding user-requested VLNVs that are not under
the "system" or top core's explicit dependency tree. The intention of
this argument is to allow users to specify cores that provide the
implementations of virtual VLNVs in these "side" trees. The CLI option
allows users to provide these without requiring someone to write a core
file with the explicit deps, so targets may be reused across multiple
different configurations, and users don't need to have duplicate core
files that only differ by virtual providers.

Signed-off-by: Alexander Williams <awill@opentitan.org>
@a-will a-will force-pushed the add-virt-providers branch from f9182be to 8003cbe Compare January 30, 2025 21:50
@a-will
Copy link
Contributor Author

a-will commented Jan 30, 2025

@olofk Here's a potential implementation of that command-line option I was talking about (for providing more dynamic insertion of deps to resolve virtual core references).

@HU90m
Copy link

HU90m commented Jan 31, 2025

@a-will a-will mentioned this pull request Jan 31, 2025
logger.warning(
"Non-deterministic selection of virtual core {} selected {}".format(
virtual[1], virtual[0]
if virtual[0] not in requested_cores:
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This matching process appears to be too strict. A request for a VLNV without the version specified doesn't match against a VLNV with the version included.

@olofk
Copy link
Owner

olofk commented Feb 1, 2025

As mentioned in #728 I think we should sync up the virtual handling for consistency reasons. In the end I would like to see support for virtual->concrete mapping in a config file, on the command-line and in the target sections.

I'm also a bit sceptical about calling this add-vlnv and allowing to throw in cores that aren't necessarily in the dependency tree. From my understanding, the purpose of this functionality is not really to add extra VLNVs, but to guide FuseSoC towards using a particular concrete implementation of a virtual. I'm a bit worried about people abusing this to actually bring in extra cores that are completely outside of the dependency tree and that this could potentially cause issues further along.

So I would suggest something like --virtual some:virtual:vlnv=a:concrete:implementation:0.2 and then we might be able to use the business logic of #728 to load this mapping from the command-line instead of from a config file.

@a-will
Copy link
Contributor Author

a-will commented Feb 1, 2025

As mentioned in #728 I think we should sync up the virtual handling for consistency reasons. In the end I would like to see support for virtual->concrete mapping in a config file, on the command-line and in the target sections.

I'm also a bit sceptical about calling this add-vlnv and allowing to throw in cores that aren't necessarily in the dependency tree. From my understanding, the purpose of this functionality is not really to add extra VLNVs, but to guide FuseSoC towards using a particular concrete implementation of a virtual. I'm a bit worried about people abusing this to actually bring in extra cores that are completely outside of the dependency tree and that this could potentially cause issues further along.

This is a fair concern. We'll maybe need to think about how to get all the power we want from --add-vlnv without the downsides.

So I would suggest something like --virtual some:virtual:vlnv=a:concrete:implementation:0.2 and then we might be able to use the business logic of #728 to load this mapping from the command-line instead of from a config file.

The downside to this exact approach is that it requires explicitly handling every virtual VLNV individually. There is no straightforward way to collect a group of them into a core representing a library of them and succinctly adding the library of providers on the command-line.

Perhaps this is where a lockfile fragment or something could come in, though. Maybe what makes --add-vlnv and virtual provider libraries get into trouble is representing the mapping as a fusesoc core (i.e. a new VLNV for the virtual provider library).

If we are to use a different kind of file to describe such a virtual provider library, then we'd need the ability to specify multiple mapping files to use (multiple libraries), and the paths to the mapping files would be a necessary argument, I think.

@olofk
Copy link
Owner

olofk commented Feb 3, 2025

The downside to this exact approach is that it requires explicitly handling every virtual VLNV individually. There is no straightforward way to collect a group of them into a core representing a library of them and succinctly adding the library of providers on the command-line.

Ok, so for my understand, was your idea is to bring in an extra VLNV that depends on all the concrete implementations to have them all set at once? If so, wouldn't this also bring in unused dependencies as well?

Perhaps this is where a lockfile fragment or something could come in, though. Maybe what makes --add-vlnv and virtual provider libraries get into trouble is representing the mapping as a fusesoc core (i.e. a new VLNV for the virtual provider library).

If we are to use a different kind of file to describe such a virtual provider library, then we'd need the ability to specify multiple mapping files to use (multiple libraries), and the paths to the mapping files would be a necessary argument, I think.

I think this is the way forward. Let's extend #728 to allow loading a custom lockfile (or probably multiple, because, why not?) that you can use for virtual->concrete mapping.

@andreaskurth
Copy link

I'm not sure lockfiles are the right place to implement the feature that is proposed in this PR.

The use case that I think won't work well goes along the following lines: As a FuseSoC user, I want to select VLNV implementation providers when I invoke FuseSoC, without having to modify the code base (including .core or .lock files). As a concrete example, consider the case in which I have a code base and would like to use one set of VLNV implementation providers when I build an FPGA bitstream, another set when I run top-level simulations with tech-specific models, and a third set when I run simulations with generic models.

If this was implemented through lockfiles, I'd have to select different combinations of lockfiles for each of the runs, right? This could work but would be less convenient than the command line argument proposed in this PR.

To select implementation providers that are not part of the code base (e.g., proprietary implementations of modules that have a generic open-source implementation), I'd even have to modify parts of the lockfiles, right? In contrast, with the command line argument proposed in this PR, I'd just have to make sure the local implementation providers can be found by FuseSoC and tell it to prefer them with a command line argument.

@a-will
Copy link
Contributor Author

a-will commented Feb 3, 2025

Ok, so for my understand, was your idea is to bring in an extra VLNV that depends on all the concrete implementations to have them all set at once? If so, wouldn't this also bring in unused dependencies as well?

That's right! Bringing in unused dependencies was a bad side effect of this approach, haha. For OT, it ended up not being too terrible because we are only using virtual VLNVs for leaf nodes in the tree--This is all for targeting different technology libraries with common interfaces. 😄

I think this is the way forward. Let's extend #728 to allow loading a custom lockfile (or probably multiple, because, why not?) that you can use for virtual->concrete mapping.

Sounds good to me! The reason for multiple files is to allow reusing descriptions of libraries of virtual->concrete maps.

In OT, we have collections of virtual cores that represent the technology library (which we call "prim libraries"). Each file representing a set of virtual->concrete maps can be such a library. In the OT repo, we can check in these files, and the user can point a fusesoc run to the desired file that was manually written to use our virtual maps.

Having the ability to specify multiple files lets someone else do a similar concept for whatever virtual cores they come up with, and OT and their repo can coexist / be used in the same design.

I will say that using file paths as arguments for inputs seems kind of different for fusesoc, which tries to discover all the things and give the user the ability to reference things by "name." A different option would be to have virtual maps as a first-class concept with names (VLNVs again?), and the user could add these named maps instead of direct file paths.

I don't have strong opinions on one implementation vs. the other.

@olofk
Copy link
Owner

olofk commented Feb 4, 2025

@andreaskurth After I had understood the use-case better I agree that we need to be able to do this on the command-line. I do however think we will have the possibility to do that now. The original proposal from @a-will was to a) create a wrapper .core file that depended on all the concrete implementations for each specific primlib and b) use the proposed --add-vlnv to add that core to the dependency graph and thus force FuseSoC to use those impls for the requested proposals. I.e. one "config" file (the wrapper .core file) and one CLI arg (--add-vlnv).

The latest suggestion is instead to a) create .lock file (that only contains virtual->concrete mappings, no actual version locks) for each primlib and b) add a --lockfile CLI arg to point out the lockfile of interest. That would also be one "config" file (the lock file) and one CLI arg (--lockfile) but it would avoid the side-effects of pulling in unnecessary deps and the potential abuse of the dependency graph. By allowing multiple lock files, we can hopefully cater to more complex flows as well.

I do understand that this is not something that normally goes into lock files in other languages, but I still think this is extremely similar in scope (locking a specific implementation or locking a specific version) that I'd say it makes sense to have this as a part of the lock file functionality. @a-will pointed out that we don't normally point out file locations on the FuseSoC command-line, but we do have functions to set locations for cores-root, build-root, log-file, config file etc, so I think it's fine this way.

Now there are a couple other things to note here.

  1. If we didn't have the requirement of allowing secret implementations of virtual cores I would had suggested using flags to control this instead to avoid the virtual handling in the first place.
  2. If you know at the top-level what concrete implementations you need (e.g. for an FPGA implementation where you really only have one choice), you can make sure these gets used without lock files, just by adding them as dependencies in a fileset that gets pulled in by that target. I'm also open for adding a key to the target section to set virtuals directly. I believe that in most cases, the target is implicitly deciding which tech to use.

@a-will
Copy link
Contributor Author

a-will commented Feb 4, 2025

Now there are a couple other things to note here.

  1. If we didn't have the requirement of allowing secret implementations of virtual cores I would had suggested using flags to control this instead to avoid the virtual handling in the first place.
  2. If you know at the top-level what concrete implementations you need (e.g. for an FPGA implementation where you really only have one choice), you can make sure these gets used without lock files, just by adding them as dependencies in a fileset that gets pulled in by that target. I'm also open for adding a key to the target section to set virtuals directly. I believe that in most cases, the target is implicitly deciding which tech to use.

For what it's worth, it's not only about secret implementations. Because of the common prim library interfaces, OT's fusesoc targets in one core file become retargetable! For example, at the block level, we can simulate a block against a pure behavioral implementation (our generic prim library) or against primitives designed for a specific technology.

Someone could come along and design a prim library based on the open source skywater PDK, for example, check that all the IPs simulate properly with the same tests we use for the generic prims, then use the validated IPs + prims in a much larger design to create a real chip. At least the IP block level would use exactly the same fusesoc targets, regardless of which prim library is chosen. Because we want these to be retargetable, we can't hard-code the concrete implementations.

Moreover, the possible set of implementations isn't limited to what we can hard-code in the fusesoc core files upstream. The downstream implementations aren't necessarily secret; they simply are allowed to multiply without needing upstream to know they exist. Thus does fusesoc achieve more of the mission to create an ecosystem of reusable IPs. 😄

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.

4 participants