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

Emit a warning when an Assign expression is not added to a domain #423

Open
kbeckmann opened this issue Jul 6, 2020 · 4 comments
Open

Emit a warning when an Assign expression is not added to a domain #423

kbeckmann opened this issue Jul 6, 2020 · 4 comments
Milestone

Comments

@kbeckmann
Copy link
Contributor

@kbeckmann kbeckmann commented Jul 6, 2020

Assigning a signal has to be done in a domain, e.g. comb: m.d.comb += signal.eq(expression), however it is easy to forget and just write signal.eq(expression). This will lead to a silent error as no warning is produced.

@whitequark
Copy link
Member

@whitequark whitequark commented Jul 6, 2020

For context, I tried implementing this and it didn't work out well due to the way "fragment transformers" are currently used. We should get rid of those and then implement this.

@awygle
Copy link
Collaborator

@awygle awygle commented Nov 7, 2020

Is this still the case?

@whitequark
Copy link
Member

@whitequark whitequark commented Nov 7, 2020

Sadly, yes. Most of the AST transformation stuff should be ripped out and replaced with a fairly typical lexical scope mechanism. That would make elaboration several times faster, make local clock domains and module flattening actually reliable, and avoid needlessly creating Assign statements that are then deleted and create spurious warnings.

That is a lot of work though. :/ We could probably also add a workaround that ignores Assigns created by nMigen itself by looking at the path or something.

@whitequark whitequark added this to the 0.4 milestone Nov 8, 2020
@whitequark
Copy link
Member

@whitequark whitequark commented Nov 8, 2020

Okay, @BracketMaster relays it from the folks at Chip11 that this issue was the biggest pain point for them while learning. I think that's a solid indication that I should just add a workaround without waiting for the massive refactor to happen. Let's fix this in 0.4.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
3 participants