-
Notifications
You must be signed in to change notification settings - Fork 58
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
Bikeshed: design for .get_fragment() #9
Comments
@jordens Any ideas on naming here? |
IMO One other thing that I noticed when trying to port the CORDIC in misoc to nmigen, that may tie into this refactoring: Subclassing a core in nmigen seems hard or impossible. Previously in migen, I'd just implement a core wrapping some functionality of another core by inheriting and then intercepting |
I don't like
I think I understand what you mean but I'd appreciate an example as it would let me see a solution more clearly. I definitely acknowledge that subclassing may be more difficult in nmigen. |
class And(Module):
def __init__(self):
self.i0 = Signal()
self.i1 = SIgnal()
self.o = Signal()
###
self.sync += self.o.eq(self.a & self.b)
class Nand(And):
def __init__(self):
super().__init__()
###
self.o, o_inner = Signal(), self.o
self.comb += self.o.eq(~o_inner) This could be done in a different way here. But the basic pattern of providing modified functionality through a similar interface by intercepting inputs and outputs is what I am looking for. |
(I assume you meant to use Can you explain why this pattern is preferable to having a submodule? Is it just because you have to forward some of the signals in the wrapper, or is there a deeper reason? |
No need to forward. It's less code, shorter and easier to read verilog. But more importantly |
You can set
Is subclassing like that really the most appropriate way to solve this? What about having a single base class that computes an intermediate result, and two subclasses that bring out different final results? (Or a single class with an argument to the constructor, which is effectively equivalent, but arugably more unwieldy, especially with a larger amount of variants than two.) That's how I would write this code in the first place in oMigen as well--using this pattern would not really occur to me, as it is IME not common in Python and it violates LSP. |
If one of the subclasses is the identity (as in these two examples) that's a lot of low density mapping code again. And while LSP is maybe violated by the example above, it isn't in the CORDIC example where the four quadrant cordic strictly extends the applicability of the two quadrant cordic. But anyway LSP violation doesn't seem relevant to the question how to subclass here. def get_fragment(self, platform):
m = Module()
m.submodules += super().get_fragment(platform)
return m.lower() |
Is it? You have something like: class And(Common):
def __init__(self):
super().__init__(self)
self.o = self.intermediate which does not seem to me like a lot of code. I.e. only the things that would be changed in the other subclass would need to be brought out.
Yes, fragments can be added as submodules.
After this discussion I think that |
On the name, there are also |
I think I like |
I think it's
That begs the question: Are fragments also modules? Functionally and/or semantically?
Yes. Getting access to the base class
|
I'm not convinced it's actually bad. If nothing else, it makes the signals that are being overridden explicit. I would like to see that written out when working on such code.
Anything that is "fragment-compatible" may be added as a submodule. That is, anything with This answers your other question as well, I believe. |
Ok. |
Point. Suggestions for better naming? |
Why do we need both This would also solve the nuisance of currently being able to add ports to a |
The naive |
You should never manually call
We agreed to rename |
Ok. The new terminology is then: Implementing |
SGTM. |
Then I have the usual |
|
It's a bit longer, but perhaps |
Or just the shorter and slightly idiosyncratic |
I know @jordens expressed in the past a desire to simplify the boilerplate around
.get_fragment
. Also, there is currently bothModule.lower
andModule.get_fragment
(aliased to one another), which is confusing and unnecessary. Also#2, Migen has aModule.get_fragment()
and combining Migen modules with nMigen modules gets even more confusing with that.The design I see for the function currently named
.get_fragment()
is as follows:.get_fragment(platform)
(which is iterated to a fixed point, i.e. a fragment, when a fragment is required).Fragment.get(obj, platform)
function that iterates an object to a fixed point with.get_fragment(platform)
or fails in a nice way.Modules are in a somewhat weird space here. Modules should be synthesizable so that you can replace:
with:
that is without having to do downstream changes after replacing a
Module
with a user class. This means that in principle you can directly return a module, likereturn m
.But this means two things:
get_fragment
call for that module is delayed. The conversion of a module to a fragment is mostly (but not entirely) error-free, so delaying this call means some errors lose context. I think it might be more useful to makeModule.get_fragment
actually never fail, and permitreturn m
.get_fragment
on a submodule, it gets aModule
object with the DSL enabled, as opposed to aFragment
object. So it can potentially abuse the DSL that way. This isn't very likely so maybe we should just ignore it.And finally, there is an important question of what to rename
.get_fragment
too. I don't want something like.build
because it is ambiguous and prone to collisions. What about.synthesize
?The text was updated successfully, but these errors were encountered: