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

submodules are allowed inside If blocks #453

Closed
jeanthom opened this issue Jul 29, 2020 · 4 comments
Closed

submodules are allowed inside If blocks #453

jeanthom opened this issue Jul 29, 2020 · 4 comments
Labels

Comments

@jeanthom
Copy link

@jeanthom jeanthom commented Jul 29, 2020

This code looks sketchy to me, but it's currently being allowed by nMigen:

from nmigen import *
from nmigen.cli import main

class MyElab(Elaboratable):
	def __init__(self):
		...

	def elaborate(self, platform):
		m = Module()

		return m

class Top(Elaboratable):
	def __init__(self):
		...

	def elaborate(self, platform):
		m = Module()

		with m.If(1):
			m.submodules += MyElab()

		return m

if __name__ == "__main__":
	top = Top()
	main(top)
@whitequark
Copy link
Member

@whitequark whitequark commented Jul 30, 2020

I'm not sure how confusing it is. I'm hesitant to forbid it outright because right now nMigen doesn't really have abstraction mechanisms besides submodules (i.e. there is no other construct similar to a "function"). If you had an actual bug caused by this I would be more inclined to make it an error, but so far I'm not aware of anyone hitting it.

@jeanthom
Copy link
Author

@jeanthom jeanthom commented Jul 30, 2020

I think it might be misleading to people who aren't familiar with HDL, thinking that they can add/remove elaboratables dynamically. By the way the generated RTLIL code looks correct (it will instantiate MyElab no matter the value of the condition).

@whitequark
Copy link
Member

@whitequark whitequark commented Jul 30, 2020

A lot of the things would be misleading to people who aren't familiar with HDL, and if we removed all of them, we wouldn't have any HDL at all. There needs to be a stronger argument, like someone actually hitting the problem.

@jeanthom
Copy link
Author

@jeanthom jeanthom commented Jul 30, 2020

No stronger argument ATM, I'm closing this issue.

@jeanthom jeanthom closed this Jul 30, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
2 participants