phanrahan / magma

magma circuits
Other
239 stars 22 forks source link

Two invocations of m.compile with different circuits (but same name) produces unexpected result #807

Open leonardt opened 3 years ago

leonardt commented 3 years ago

This example

# a.py
import magma as m

class FullAdder(m.Circuit):
    io = m.IO(
        a=m.In(m.Bit),
        b=m.In(m.Bit),
        cin=m.In(m.Bit),
        sum_=m.Out(m.Bit),
        cout=m.Out(m.Bit)
    )
    io.sum_ @= io.a ^ io.b ^ io.cin
    io.cout @= (io.a & io.b) | (io.b & io.cin) | (io.a & io.cin)

class Adder(m.Generator2):
    def __init__(self, n: int):
        self.io = io = m.IO(
            A=m.In(m.UInt[n]),
            B=m.In(m.UInt[n]),
            CIN=m.In(m.Bit),
            SUM=m.Out(m.UInt[n]),
            COUT=m.Out(m.Bit)
        )
        curr_cin = io.CIN
        for i in range(n):
            next_sum, curr_cin = FullAdder()(io.A[i], io.B[i], curr_cin)
            io.SUM[i] @= next_sum
        io.COUT @= curr_cin

for i in [4, 16]:
    file = f"Adder{i}"
    m.compile(file, Adder(i), inline=True)

produces two files Adder4.v and Adder16.v, but they both contain the same contents (i.e. the Adder(4) definition of the Adder module is being reused in the second file).

$ python a.py
$ diff Adder4.v Adder16.v

I suspect the problem is that we're not uniquifying names across invocations of m.compile, and this means that coreir is using the old definition for the Adder circuit when we lookup the module by name.

rsetaluri commented 3 years ago

Yes, that suspicion is correct. I will revisit uniquification/caching/coreir context. In the meantime, if this is blocking you can we reset the CoreIR context manually in between?

leonardt commented 3 years ago

Yea, it's not blocking, just noted it so we have it documented. It came up as I was writing a fault test that calls compile and run twice, which seemed like something that might be okay to do as a new user, so ideally we can fix this to make onboarding easier.