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

lib.coding: remove GrayDecoder comb loop for consistency #726

Merged
merged 1 commit into from Nov 3, 2022

Conversation

widlarizer
Copy link
Contributor

While developing a prototype for #704 at 0526474, I discovered that bits of Graydecoder.o are combinationally dependent on each other. When each bit is thought of as an individual signal, this is no problem, but a comb loop detection pass would be slowed down by that assumption. Regardless of if it's within the scope of the language's "no indirect comb loops" policy, it is easily rewritable to never have assignments that have the output signal both on the left and righ-hand sides. That is what this PR contains. All tests pass, including the formal check of gray encoding reversibility, proving correctness of this PR.

@codecov-commenter
Copy link

codecov-commenter commented Oct 31, 2022

Codecov Report

Merging #726 (8170a47) into main (beb1b38) will increase coverage by 0.00%.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##             main     #726   +/-   ##
=======================================
  Coverage   81.17%   81.17%           
=======================================
  Files          49       49           
  Lines        6534     6535    +1     
  Branches     1567     1567           
=======================================
+ Hits         5304     5305    +1     
  Misses       1036     1036           
  Partials      194      194           
Impacted Files Coverage Δ
amaranth/lib/coding.py 100.00% <100.00%> (ø)

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@whitequark
Copy link
Member

I discovered that bits of Graydecoder.o are combinationally dependent on each other.

So... I'm on the fence as to whether this should be allowed. I know, for a fact, that people write such code and expect it to work. (CXXRTL, for example, has to handle this, and so does Amaranth currently.)

I do not know whether it should be allowed in Amaranth. Currently, a literal reading of the language guide prohibits such apparent combinatorial loops, so your PR follows its text; however, that section doesn't really talk about word-level combinatorial loops that disappear once you look at the bit-level. Since there was no way to detect or forbid this pattern, I'm sure that eventually forbidding it will break enough real-world code to be a concern.

@widlarizer
Copy link
Contributor Author

If you - or users - believe Amaranth has staying power in the HDL space, it might be worth it to break small parts of people's designs as soon as possible, rather than overconstraining what the implementation can do with users' designs. I mean, Amaranth is on version 0.x, that seems like a signal to users that things can shift around.

As far as this PR goes, I realized that the Values built are bigger with this change. Could be a perf issue if people decide to, um, gray decode thousands of bits, or something, not a big deal, but it's just more IR propagating until yosys does your AIG strashing or something, which doesn't even happen with pysim, if I understand it correctly.

@whitequark whitequark merged commit db24a14 into amaranth-lang:main Nov 3, 2022
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.

None yet

3 participants