dan-fritchman / Hdl21

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

Change PDK Modules `SimpleNamespace` to Submodules #147

Closed ThomasPluck closed 1 year ago

ThomasPluck commented 1 year ago

Including the digital cells alongside the primitives has a few annoying side-effects:

It'd be better to have standard cell libraries in different namespaces for these reasons and make it something as slick as #114, where we can just use:

from gf180 import digital7track as d
simple_and_gate = d.and2_1

Instead of:

from sky130 import modules as s
# Now you wait for 5 whole seconds
simple_and_gate = s.sky130_fd_sc_hdll__and2_1
dan-fritchman commented 1 year ago

Sounds great, totally agree.

ThomasPluck commented 1 year ago

So getting into the details of this issue, it seems that with the SimpleNamespace approach:

from sky130.digital import high_density as hd

Is actually logically equivalent to:

import sky130.digital as _tmp
hd = _tmp.high_density
del _tmp

Which is... horrible. This stems from the fact that SimpleNamespace is a variable type rather than a submodule. To really get fine-grained control of imports and ensure that we're only importing the cells we're interested in, we'd have to split up the files into individual submodules which are populated without the use of SimpleNamespace.

I've tried this out and works pretty well on my local machine but the call changes to:

import sky130.digital.high_density as hd

And teasing apart the primitives to mirror this change, would require effort that may not be worth it... hey, you might have 4s of unnecessary overhead when loading in your digital cells, but at least you don't have that overhead when loading in the base PDK module and the primitives (the core functionality of Hdl21).

So, going to leave this open for now, change the name to reflect the state of affairs, and mark it a "good first issue" for newcomers.

ThomasPluck commented 1 year ago

Okay was just being lazy here this got resolved in latest push of #114