[FIRRTL][AdvancedLayerSink] Add the ability to sink ops by cloning

[rwy7]

In advanced-layer-sink, add the ability to sink ops by cloning them to their uses. This PR is only cloning constant-like ops, but we have the option to clone more sorts of ops as needed.

Constants are heavily shared, so they typically aren't sinkable by movement. Cloning will put constants directly into the layerblock where they are used, which avoids having us capture the constant as a port when lowering layerblocks to modules. It is important to put constants directly in a module because it enables more canonicalization later in the pipeline.

We used to rely on IMCP to sink constants after lower-layers, but we lost that as we moved the layer lowering passes later in the pipeline, which is why we now have to clone constants ourselves.

[seldridge]

One question. Otherwise this is looking great!

[seldridge]

Is this correct to iterate through results and clone individually?

This seems like it would do something wrong with hypothetical constant operations with multiple outputs whose users are in different blocks. Does this work for the following?

%0, %1 = firrtl.aggconstant 1, 3 : () -> (i1, i2)
firrtl.layerblock @A {
  bar(%0) : (i1) -> ()
}
firrtl.layerblock @B {
  bar(%1) : (i2) -> ()
}

The right thing may be to either only work on operations which have one result or to invalidate the users of the results which are now unused.

[rwy7]

I think this code should work fine, it would create a clone for each layerblock, and replace each use with the correct result of the correct clone (ie, old result-no == new result-no). I tried it out by letting advanced-layer-sink clone instance ops, here is an example:

// ./bin/circt-opt -pass-pipeline="builtin.module(firrtl.circuit(firrtl-advanced-layer-sink))" -allow-unregistered-dialect ../scratch.mlir

firrtl.circuit "Test" {
  firrtl.layer @A bind {}
  firrtl.layer @B bind {}

  firrtl.module @Foo(out %a : !firrtl.uint<1>, out %b : !firrtl.uint<1>) {
    %c1_ui1 = firrtl.constant 1 : !firrtl.uint<1>
    firrtl.matchingconnect %a, %c1_ui1 : !firrtl.uint<1>
    firrtl.matchingconnect %b, %c1_ui1 : !firrtl.uint<1>
  }

  firrtl.module public @Test() {
    %foo_a, %foo_b = firrtl.instance foo @Foo(out a : !firrtl.uint<1>, out b : !firrtl.uint<1>)
    firrtl.layerblock @A {
      "unknown"(%foo_a) : (!firrtl.uint<1>) -> ()
      "unknown"(%foo_b) : (!firrtl.uint<1>) -> ()
    }

    firrtl.layerblock @B {
      "unknown"(%foo_a) : (!firrtl.uint<1>) -> ()
      "unknown"(%foo_b) : (!firrtl.uint<1>) -> ()
    }
  }
}

is translated to:

module {
  firrtl.circuit "Test" {
    firrtl.layer @A bind {
    }
    firrtl.layer @B bind {
    }
    firrtl.module @Foo(out %a: !firrtl.uint<1>, out %b: !firrtl.uint<1>) {
      %c1_ui1 = firrtl.constant 1 : !firrtl.uint<1>
      firrtl.matchingconnect %a, %c1_ui1 : !firrtl.uint<1>
      firrtl.matchingconnect %b, %c1_ui1 : !firrtl.uint<1>
    }
    firrtl.module public @Test() {
      %foo_a, %foo_b = firrtl.instance foo @Foo(out a: !firrtl.uint<1>, out b: !firrtl.uint<1>)
      firrtl.layerblock @A {
        %foo_a_0, %foo_b_1 = firrtl.instance foo @Foo(out a: !firrtl.uint<1>, out b: !firrtl.uint<1>)
        "unknown"(%foo_a_0) : (!firrtl.uint<1>) -> ()
        "unknown"(%foo_b_1) : (!firrtl.uint<1>) -> ()
      }
      firrtl.layerblock @B {
        %foo_a_0, %foo_b_1 = firrtl.instance foo @Foo(out a: !firrtl.uint<1>, out b: !firrtl.uint<1>)
        "unknown"(%foo_a_0) : (!firrtl.uint<1>) -> ()
        "unknown"(%foo_b_1) : (!firrtl.uint<1>) -> ()
      }
    }
  }
}

(it leaves the original instance in place because instance ops are not pure and I am using isOpTriviallyDead rather than just checking for empty uses).

[seldridge]

Yes, that seems like it works. And it will still clone if foo_a is only used in layerblock @A and foo_b is only used in layerblock @B?

This is also done with modifications to the code to weaken cloneable to allow it to clone instances, correct?

[rwy7]

And it will still clone if foo_a is only used in layerblock @A and foo_b is only used in layerblock @B?

Yep, here is an example:

firrtl.circuit "Test" {
  firrtl.layer @A bind {}
  firrtl.layer @B bind {}

  firrtl.module @Foo(out %a : !firrtl.uint<1>, out %b : !firrtl.uint<1>) {
    %c1_ui1 = firrtl.constant 1 : !firrtl.uint<1>
    firrtl.matchingconnect %a, %c1_ui1 : !firrtl.uint<1>
    firrtl.matchingconnect %b, %c1_ui1 : !firrtl.uint<1>
  }

  firrtl.module public @Test() {
    %foo_a, %foo_b = firrtl.instance foo @Foo(out a : !firrtl.uint<1>, out b : !firrtl.uint<1>)
    firrtl.layerblock @A {
      "unknown"(%foo_a) : (!firrtl.uint<1>) -> ()
    }

    firrtl.layerblock @B {
      "unknown"(%foo_a) : (!firrtl.uint<1>) -> ()
    }
  }
}

To:

module {
  firrtl.circuit "Test" {
    firrtl.layer @A bind {
    }
    firrtl.layer @B bind {
    }
    firrtl.module @Foo(out %a: !firrtl.uint<1>, out %b: !firrtl.uint<1>) {
      %c1_ui1 = firrtl.constant 1 : !firrtl.uint<1>
      firrtl.matchingconnect %a, %c1_ui1 : !firrtl.uint<1>
      firrtl.matchingconnect %b, %c1_ui1 : !firrtl.uint<1>
    }
    firrtl.module public @Test() {
      %foo_a, %foo_b = firrtl.instance foo @Foo(out a: !firrtl.uint<1>, out b: !firrtl.uint<1>)
      firrtl.layerblock @A {
        %foo_a_0, %foo_b_1 = firrtl.instance foo @Foo(out a: !firrtl.uint<1>, out b: !firrtl.uint<1>)
        "unknown"(%foo_a_0) : (!firrtl.uint<1>) -> ()
      }
      firrtl.layerblock @B {
        %foo_a_0, %foo_b_1 = firrtl.instance foo @Foo(out a: !firrtl.uint<1>, out b: !firrtl.uint<1>)
        "unknown"(%foo_a_0) : (!firrtl.uint<1>) -> ()
      }
    }
  }
}

This is also done with modifications to the code to weaken cloneable to allow it to clone instances, correct?

Right.

[youngar]

LGTM

[seldridge]

LGTM

The instance cloning has me thinking that you could eventually think about a cost model for this. cloneable is really that and it's just that constant ops are free. There may eventually be other ops we can clone if the cost model allows. Just food for thought.