dan-fritchman / Hdl21

Hardware Description Library
BSD 3-Clause "New" or "Revised" License
69 stars 16 forks source link

Generator + regular-function stack, or, elaboration caching aint right #192

Closed dan-fritchman closed 1 year ago

dan-fritchman commented 1 year ago

Test case:

def test_generator_function_combo():
    """ Test instantiating hierarchies that combine `@h.generator`s and regular old functions. """

    @h.generator
    def Gen(_: h.HasNoParams) -> h.Module:
        @h.module
        class HasDiff:
            d = h.Diff(port=True)
        return HasDiff

    def NonGen() -> h.Module:
        @h.module
        class HasTwoHasDiffs:
            d = h.Diff(port=False)
            a = Gen()(d=d)
            b = Gen()(d=d)
        return HasTwoHasDiffs

    h.elaborate(NonGen())
    h.elaborate(NonGen())

Produces:

E       RuntimeError: Elaboration Error at hierarchical path: 
E         Module        HasTwoHasDiffs              hdl21/tests/test_hdl21.py:1511
E         Instance      a                           hdl21/tests/test_hdl21.py:1513
E       Invalid connections `{'d_p': Missing connection to Port `d_p`, 'd_n': Missing connection to Port `d_n`, 'd': Connection to non-existent Port `d`}` on Instance `a` in Module `HasTwoHasDiffs`

hdl21/elab/elaborators/base.py:186: RuntimeError
dan-fritchman commented 1 year ago

A few notes:

dan-fritchman commented 1 year ago

The fact that this happens is tied to the (generally fraught) topic of flattening bundle-valued ports. More generally, of doing anything that modifies ports during an elaboration pass.

Why:

I think this bug's been there forever. The sole answer would seem to be per-pass elaboration "caches"/ "done-lists", particularly for elaboration passes which are not idempotent. I think that is only bundle-flattening to date, but there are plenty more possible (e.g. "infer unspecified port directions").

And why do the generator versions work? We can just more aggressively identify, cache, and skip re-running them. I suspect that's papering over this bug, perhaps other similar ones.