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

Add initial support for coreir-verilog instance params #463

Merged
merged 5 commits into from Oct 23, 2019

Conversation

leonardt
Copy link
Collaborator

This fixes a bug where the logic to passthrough instance parameters to verilog (<mod>(key=value)) only worked with the verilog backend. This adds support to the pattern for the coreir-verilog backend by adding an additional parameter param_map to the from verilog constructs. This is required so that the coreir definitions can be instanced with the parameters are config args (otherwise we get a Args and params are not the same! assertion failure).

This is a hotfix to add the feature, but we may want to consider an alternative solution for the longer term: Add a metadata field for passing through these verilog parameters as key/value strings. Since this is reserved for working with verilog black boxes, perhaps it's best to just treat this as sidechannel information that's just passed through the compiler, rather than converting from verilog to coreir types then back.

@rdaly525 The basic summary of the issue is: we have black box verilog that is being imported into magma. These verilog modules have parameters that we'd like to pass through when instancing (they don't affect the interface). The current workaround is to have the user explicitly declare the parameter types so they are converted to coreir config args. The other option is to have coreir have a notion of black box parameters that are just passed through to the instance of the black box verilog.

Other comments/suggestions welcome

@coveralls
Copy link

coveralls commented Oct 21, 2019

Pull Request Test Coverage Report for Build 2065

  • 24 of 31 (77.42%) changed or added relevant lines in 2 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.1%) to 74.247%

Changes Missing Coverage Covered Lines Changed/Added Lines %
magma/backend/coreir_.py 15 22 68.18%
Totals Coverage Status
Change from base Build 2056: 0.1%
Covered Lines: 8724
Relevant Lines: 11750

💛 - Coveralls

Copy link
Collaborator

@jameshegarty jameshegarty left a comment

Choose a reason for hiding this comment

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

I don’t know much about this code, but looks fine to me. At some point in the future I’d prefer passing the parameters to the generator, since imo that would make more logical sense and would allow you to have the interface chance (which is often why people need this), but I think this is fine for now :)

Copy link
Collaborator

@rsetaluri rsetaluri left a comment

Choose a reason for hiding this comment

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

Agree that this is a good solution for now. But agree with all of the comments above that we want to think of a longer term/robust solution.

@leonardt leonardt merged commit 9650855 into master Oct 23, 2019
@leonardt leonardt deleted the coreir-verilog-instance-params branch October 23, 2019 20:01
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.

None yet

4 participants