phanrahan / magma

magma circuits
Other
251 stars 23 forks source link

Wiring different subclasses of Digital is allowed #651

Closed rsetaluri closed 4 years ago

rsetaluri commented 4 years ago

the following is allowed. is that expected?

import magma as m

class Foo(m.Circuit):
    io = m.IO(I=m.In(m.Clock), O=m.Out(m.Reset))
    m.wire(io.I, io.O)

Came across this when trying to clean up magma/digital.py. It seems there are references to it's subclasses, which introduces a circ. dependency:

https://github.com/phanrahan/magma/blob/4605be27ce38413ee192c24a7d8a1c779e99bb06/magma/digital.py#L205

https://github.com/phanrahan/magma/blob/4605be27ce38413ee192c24a7d8a1c779e99bb06/magma/digital.py#L80

...among others.

Even if the first snipped is expected to be allowed, I'd like to remove the circular dependency if possible

leonardt commented 4 years ago

This is a bug, we should fix it. the circular dependency is for backwards compatibility (VCC and GND should be of type Digital, but there's existing code that relies on them being Bit, so we can remove that once we fix existing internal code, I suspect that there's not much user code that relies on this though so that's not as much of a worry)

rsetaluri commented 4 years ago

Got it. Taking a look at this code so I will try to kill both birds with one stone.

Can you point to code that depends on VCC/GND being a Bit?

leonardt commented 4 years ago

Related discussion: https://github.com/phanrahan/magma/issues/499, revert PR: https://github.com/phanrahan/magma/pull/536

The old behavior of Bit(0) and Bit(1) was to return VCC and GND. Here's one place that it's used: https://github.com/phanrahan/magma/blob/master/magma/backend/coreir_transformer.py#L221

Basically, there's code that relies on is VCC or is GND to check constants, which should be changed to check it another way.

I think changing VCC/GND to Digital makes sense semantically, and having Bit(0) and Bit(1) be different (just constants Bit instances) would make the most sense.

rsetaluri commented 4 years ago

Got it, makes sense. What about constant values of Clock/Reset/etc.?

I see this from Pat:

Thinking about this in hindsight, I think we should depreciate VCC and GND for values of Bit.

rsetaluri commented 4 years ago

@leonardt take a look at bit.py, digital.py in https://github.com/phanrahan/magma/pull/652/

(some other stuff in there, but trying to clean up circular dependencies, and blanket package imports)