Bikeshed: design for .get_fragment()

[nmigen-issue-migration]

Issue by whitequark
Monday Dec 17, 2018 at 20:57 GMT
Originally opened as m-labs/nmigen#9

---

I know @jordens expressed in the past a desire to simplify the boilerplate around .get_fragment. Also, there is currently both Module.lower and Module.get_fragment (aliased to one another), which is confusing and unnecessary. Also#2, Migen has a Module.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:

• There are 2 kinds of synthesizable entities in nMigen: fragments (which are directly synthesizable) and anything that has .get_fragment(platform) (which is iterated to a fixed point, i.e. a fragment, when a fragment is required).
• Instances, memory ports, etc, are synthesizable. Which class they actually are is an implementation detail that is subject to change. (Instances are currently just fragments, memory ports are significantly more complex due to some mismatch between Yosys and Migen representation of ports, etc).
• There is a 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:

sm = Module()
# ... operate on sm
m = Module()
m.submodules += sm

with:

class SubCircuit:
    def get_fragment(self, platform):
        m = Module()
        return m.lower(platform)
sm = SubCircuit()
m = Module()
m.submodules += sm

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, like return m.

But this means two things:

1. The actual 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 make Module.get_fragment actually never fail, and permit return m.
2. If a parent module directly calls get_fragment on a submodule, it gets a Module object with the DSL enabled, as opposed to a Fragment 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?

[nmigen-issue-migration]

Comment by whitequark
Sunday Jan 13, 2019 at 08:53 GMT

---

@jordens Any ideas on naming here?

[nmigen-issue-migration]

Comment by jordens
Tuesday Jan 15, 2019 at 12:21 GMT

---

IMO .synthesize() would also colide with what the lower toolchain layers claim to do.
.elaborate() maybe?

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 Signal()s as needed (see TwoQuadrantCORDIC vs the full CORDIC that remaps the quadrants).
This is now hard because I was unable to figure out a workable way to intercept/override/remap Signals in __init__ and at the same time properly overload and handle get_fragment() from the two classes. If this explanation is too convoluted I'll come up with a minimal example that shows what I'd like to do.

[nmigen-issue-migration]

Comment by whitequark
Tuesday Jan 15, 2019 at 12:23 GMT

---

I don't like .elaborate() but I don't have any options I like so it's probably fine.

If this explanation is too convoluted I'll come up with a minimal example that shows what I'd like to do.

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.

[nmigen-issue-migration]

Comment by jordens
Tuesday Jan 15, 2019 at 13:12 GMT

---

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.
In nmigen I would have to own the inner And in submodules and then add i0, i1 as attributes to Nand (or forward them with jet more signals).

[nmigen-issue-migration]

Comment by whitequark
Tuesday Jan 15, 2019 at 13:16 GMT

---

(I assume you meant to use super() above.)

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?

[nmigen-issue-migration]

Comment by jordens
Tuesday Jan 15, 2019 at 13:32 GMT

---

No need to forward. It's less code, shorter and easier to read verilog. But more importantly Nand doesn't need to know anything about the input interface (how many and where the inputs are and how they are named). All those also apply to the original CORDIC case.

[nmigen-issue-migration]

Comment by whitequark
Tuesday Jan 15, 2019 at 13:40 GMT

---

shorter and easier to read verilog

You can set f.flatten on a fragment to get the same Verilog, at least.

But more importantly Nand doesn't need to know anything about the input interface (how many and where the inputs are and how they are named). All those also apply to the original CORDIC case.

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.

[nmigen-issue-migration]

Comment by jordens
Tuesday Jan 15, 2019 at 14:12 GMT

---

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.
In addition to the signal interception question: How do you overload get_fragment()? Can I add more logic to whatever (Fragment or Module) I obtain from super().get_fragment()? Is .lower() idempotent?
I think there is a functional problem with the semantics of the Fragment/Module split. Can I add a fragment as a submodule?

def get_fragment(self, platform):
    m = Module()
    m.submodules += super().get_fragment(platform)
    return m.lower()

[nmigen-issue-migration]

Comment by whitequark
Tuesday Jan 15, 2019 at 14:16 GMT

---

that's a lot of low density mapping code again

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.

I think there is a functional problem with the semantics of the Fragment/Module split. Can I add a fragment as a submodule?

Yes, fragments can be added as submodules.

In addition to the signal interception question: How do you overload get_fragment()? Can I add more logic to whatever (Fragment or Module) I obtain from super().get_fragment()? Is .lower() idempotent?

After this discussion I think that get_fragment() should return a Module i.e. not call lower. (This is actually easier than the current behavior.) So yes, you could extend the returned module in a subclass.

[nmigen-issue-migration]

Comment by jordens
Tuesday Jan 15, 2019 at 14:16 GMT

---

On the name, there are also .emit(), .refine(), .specialize(), .generate().

[nmigen-issue-migration]

Comment by whitequark
Tuesday Jan 15, 2019 at 14:17 GMT

---

I think I like elaborate the most here, because it is clearly HDL-specific, even if the meaning is somewhat different from what Verilog has.

[nmigen-issue-migration]

Comment by jordens
Tuesday Jan 15, 2019 at 14:27 GMT

---

that's a lot of low density mapping code again

Is it? You have something like:
[...]
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.

I think it's super().__init__() without the self.
Yes. I meant those four lines of extra code. That could arguably be made smaller for complicated interfaces with many Signals by the use of Records. But to force the use of a Record just because it makes implementing an identity wrapper easier seems backwards.

I think there is a functional problem with the semantics of the Fragment/Module split. Can I add a fragment as a submodule?

Yes, fragments can be added as submodules.

That begs the question: Are fragments also modules? Functionally and/or semantically?

After this discussion I think that get_fragment() should return a Module i.e. not call lower. (This is actually easier than the current behavior.) So yes, you could extend the returned module in a subclass.

Yes. Getting access to the base class Module() in subclasses generally seems desirable.
Then if get_fragment() returns a Module, who should call lower()?

.modularize() or even .get_module()?

[nmigen-issue-migration]

Comment by whitequark
Tuesday Jan 15, 2019 at 14:31 GMT

---

Yes. I meant those four lines of extra code.

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.

That begs the question: Are fragments also modules? Functionally and/or semantically?

Anything that is "fragment-compatible" may be added as a submodule. That is, anything with get_fragment() function. The only constraint on get_fragment() is that it should return another fragment-compatible object; during lowering, this is iterated to fixed point. That is how a get_fragment() function can delegate to platform.get_primitive() which might want to return a Module or even another fragment-compatible object.

This answers your other question as well, I believe.

[nmigen-issue-migration]

Comment by jordens
Tuesday Jan 15, 2019 at 14:38 GMT

---

Ok. Modules are fragment-compatible. But the naming of submodule is at least confusing as it indicates the reverse is also true (Fragment being added as a submodule).

[nmigen-issue-migration]

Comment by whitequark
Tuesday Jan 15, 2019 at 14:39 GMT

---

Point. Suggestions for better naming?