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

make Record inherit from UserValue #357

Merged
merged 1 commit into from Apr 16, 2020
Merged

make Record inherit from UserValue #357

merged 1 commit into from Apr 16, 2020

Conversation

anuejn
Copy link
Contributor

@anuejn anuejn commented Apr 15, 2020

closes #354

nmigen/hdl/rec.py Show resolved Hide resolved
nmigen/back/rtlil.py Outdated Show resolved Hide resolved
@codecov
Copy link

codecov bot commented Apr 15, 2020

Codecov Report

Merging #357 into master will increase coverage by 0.14%.
The diff coverage is 91.66%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #357      +/-   ##
==========================================
+ Coverage   82.36%   82.50%   +0.14%     
==========================================
  Files          35       35              
  Lines        5846     5950     +104     
  Branches     1205     1210       +5     
==========================================
+ Hits         4815     4909      +94     
- Misses        868      873       +5     
- Partials      163      168       +5     
Impacted Files Coverage Δ
nmigen/back/pysim.py 91.04% <ø> (+0.03%) ⬆️
nmigen/hdl/xfrm.py 96.24% <ø> (+0.10%) ⬆️
nmigen/back/rtlil.py 81.57% <75.00%> (-0.62%) ⬇️
nmigen/hdl/ast.py 88.77% <100.00%> (+0.16%) ⬆️
nmigen/hdl/rec.py 96.91% <100.00%> (-0.61%) ⬇️
nmigen/tracer.py 89.18% <0.00%> (-5.41%) ⬇️
nmigen/hdl/ir.py 95.16% <0.00%> (-0.24%) ⬇️
nmigen/hdl/cd.py 100.00% <0.00%> (ø)
... and 15 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 3346f2c...4a448ea. Read the comment docs.

@anuejn
Copy link
Contributor Author

anuejn commented Apr 15, 2020

Another thing that came to my mind just now is, that we maybe want to somehow mark "things that are somehow a bundle". This would help to implement genlib.record.iter_flat() without the assumption that Record is the only bundle. Do you have any ideas of how to achive that?

@whitequark
Copy link
Member

This would help to implement genlib.record.iter_flat() without the assumption that Record is the only bundle. Do you have any ideas of how to achive that?

As described elsewhere, I don't think this is a good idea, and nMigen's built-in records will not support iter_flat. (They are still there in nmigen.compat, but only for compatibility.)

@anuejn
Copy link
Contributor Author

anuejn commented Apr 15, 2020

okay

@whitequark
Copy link
Member

To add to this, I think it is important to examine the use cases of iter_flat and see which other mechanism would suit them well. Of course, it is also important to have an escape hatch for those who really want to use their own kind of records in their own codebase, which is why UserValue is there.

But I don't currently see a convincing case for having a core interoperability mechanism for flat iteration.

@anuejn
Copy link
Contributor Author

anuejn commented Apr 15, 2020

I have some utility function to measure the size of a Module (/Elaboratable) by synthesizing it with Yosys and pulling some output from the stat command. To generate the rtlil for this, I use a similar function to extract all nMigen Value like things and add them to the io ports.

But maybe this usecase is a bit too unimportant / hacky to be supported ;).

@whitequark
Copy link
Member

To generate the rtlil for this, I use a similar function to extract all nMigen Value like things and add them to the io ports.

Take a look at the Yosys expose command. It will let you do this without any cooperation from nMigen at all.

@anuejn
Copy link
Contributor Author

anuejn commented Apr 15, 2020

ah nice thanks :)

@whitequark
Copy link
Member

Oh, and although the Yosys expose command is probably the most elegant way to do this, you're not limited to using it; you can convert the Module into a Fragment, and then iterate through stmt._rhs_signals() for every statement. Really, your use case has quite a few ways to solve it, none of which involve iter_flat as a core interoperability mechanism.

nmigen/back/rtlil.py Outdated Show resolved Hide resolved
nmigen/hdl/ast.py Outdated Show resolved Hide resolved
Copy link
Member

@whitequark whitequark 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, should be OK to merge with the test added.

self.__lowered = Value.cast(self.lower())
lowered = self.lower()
if isinstance(lowered, UserValue):
lowered = lowered._lazy_lower()
Copy link
Member

Choose a reason for hiding this comment

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

Could you please write a test for lowering of a UserValue that returns another UserValue?

Copy link
Member

@whitequark whitequark left a comment

Choose a reason for hiding this comment

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

LGTM

@whitequark whitequark merged commit ff6c032 into amaranth-lang:master Apr 16, 2020
@whitequark
Copy link
Member

Thanks for the PR!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

Inherit Record from UserValue
2 participants