[RFC] Disable automatic clock wiring logic when multiple clock drivers are present

[leonardt]

Right now, magma takes the first clock driver it finds in the interface and uses it as the automatic driver for unwired clocks. The proposal is to raise an error if there is more than one possible default driver, rather than try to guess the default. This would actually catch the error in the example from https://github.com/phanrahan/magma/issues/898 where there are two possible defaults (we would not be able to choose the right driver, and coreir would complain about an undriven signal), but I still think we should do the more general improvement mentioned there about handling errors.

I don't think this is a breaking change since I'm not aware of any code that relies on this logic in a multiple clock situation. However, the current logic introduces a certain class of bugs, with this example specifically found in the facebook design:

class Foo(m.Circuit):
    io = m.IO(clk=m.In(m.Clock), ...)
    clock_gate_inst = ClockGateLogic()
    clock_gate_inst.clk_in @= io.clk

    other_inst = OtherLogic()
    # forgot to wire other_inst.clk @= clock_gate_inst.clk_out

What happens is since the user forgot to wire up the clk driver for other_inst, it receives the default driver from the interface io.clk. This was rather difficult to catch because (a) magma didn't raise an error about an undriven clock and (b) the default clock driver logic still works from a verification standpoint, so the only way to find it is a detailed QoR study to find that the clock gating was not improving the quality.

This example adds a wrinkle in that the second possible clock driver comes from an instance, not an interface port, so the default driver logic needs to be updated to recognize this possibility. The proposal again in this example would be to have the compiler raise an error because it cannot resolve a default clock driver.

From my understanding: FB uses explicitly wired clocks everywhere (so they are okay with this change) and garnet uses a single clock everywhere (unless we added clock gating at the magma level, which would mean that they would likely benefit from this change and any errors introduced were silently missed before)

[rsetaluri]

This seems good. Just so I understand, the proposal is for the clock wiring pass to look like (pseudo-code):

def wire_clocks(defn):
    clks = find_clocks(defn.interface)  # list of all clock types
    if len(clks) != 1: return
    clk = clks[0]
    for inst in defn.instances:
        clks = [c if is_unwired(c) for c in find_clocks(inst)]
        for inst_clk in clks:
            inst_clk @= clk

If so, I think this is a good plan.

[leonardt]

Fixed by https://github.com/phanrahan/magma/commit/c9ce5a4d7e542bfbae2e497e7a6456edd872038c