[FIRRTL] Make IMDCE work for ops w/ regions/blocks

[seldridge]

Fix a bug in FIRRTL's IMDCE Pass where it would not visit the blocks of operations. This can result in crashes for blocks which contain users of FIRRTL modules, e.g., instances inside layerblocks of sv.ifdef.

This conceptually is two changes: (1) when marking a block live, the block needs to be recursively walked and (2) when erasing ops, a recursive walk is needed.

[fabianschuiki]

LGTM! Nice mechanical change.

[youngar]

Feel free to ignore the comments, I'm not sure they are valid.

[youngar]

Could this work as a preorder walk? If you have a dead op with a region, there is no reason to rewrite the region if the whole thing is going to be deleted anyways. We don't have any ops like this right now

[seldridge]

It could be, however, this would require us to better define whether or not all the ops in the FIRRTL dialect have side effects. Consider the following example:

firrtl.circuit "Foo" {
  firrtl.layer @A bind {
  }
  firrtl.module private @Bar(in %clock: !firrtl.clock, in %a: !firrtl.uint<1>, out %b: !firrtl.probe<uint<1>, @A>, out %c: !firrtl.uint<1>) {
    %0 = firrtl.not %a : (!firrtl.uint<1>) -> !firrtl.uint<1>
    firrtl.matchingconnect %c, %0 : !firrtl.uint<1>
    firrtl.layerblock @A {
      %1 = firrtl.not %a : (!firrtl.uint<1>) -> !firrtl.uint<1>
      %a_not = firrtl.node %1 : !firrtl.uint<1>
      %2 = firrtl.ref.send %a_not : !firrtl.uint<1>
      %3 = firrtl.ref.cast %2 : (!firrtl.probe<uint<1>>) -> !firrtl.probe<uint<1>, @A>
      firrtl.ref.define %b, %3 : !firrtl.probe<uint<1>, @A>
    }
  }
  firrtl.module @Foo(in %clock: !firrtl.clock, in %a: !firrtl.uint<1>, out %c: !firrtl.uint<1>) attributes {convention = #firrtl<convention scalarized>} {
    %bar_clock, %bar_a, %bar_b, %bar_c = firrtl.instance bar @Bar(in clock: !firrtl.clock, in a: !firrtl.uint<1>, out b: !firrtl.probe<uint<1>, @A>, out c: !firrtl.uint<1>)
    firrtl.matchingconnect %bar_clock, %clock : !firrtl.clock
    firrtl.matchingconnect %bar_a, %a : !firrtl.uint<1>
    firrtl.matchingconnect %c, %bar_c : !firrtl.uint<1>
  }
}

This has a path through a layerblock via a layer-colored probe. I have an out-of-tree change that I will push that makes layerblock ops RecursiveMemoryEffects and RecursivelySpeculatable (which they arguably should be). With that change, if I do a post-order walk, then I will delete the layerblock op since it is trivially dead after all the operations are removed (which are not side effect free as defined in ODS). If I do a pre-order walk, then I will not delete the layerblock op.

Given the way that we've defined FIRRTL operations, the post-order walk seems to be the only way to do this if we want to delete the layerblock as part of this pass as opposed to relying on something else to delete it, e.g., a canonicalizer.

h/t @rwy7 for working through this example.

[seldridge]

Change to add the missing traits to layerblocks here: 8f8acc2

[seldridge]

I added a test here showing this example: 08c3c8a

[youngar]

Something seems sketchy to me about this. We don't have many ops other than FModuleOps that have block arguments, but if we did: if a block argument is DontTouch'd, shouldn't we always mark the owning operation as alive, and not if(isa<FModuleOp>(op))? That being said, hasDontTouch(value) first checks to see if the owning op is DontTouchd and then checks for port symbols and annotations iff it is an FModuleOp. It seems a bit weird to me that we say a port is dont touched if the owning module is.

[seldridge]

What would you say are the defined semantics of DontTouchAnnotation? 😉

From an annotation perspective, this can only be applied to certain named things (modules, wires, registers, memories, and ports). This annotation has the semantics of, when applied to a module, is equivalent to don't touching all the ports. Given that interpretation, then this is doing the right thing. Or: it's reasonable to expect that this is a module port and not a general block arg as this was never intended to be defined for block args, just for ports.

That said, this annotation has been begging for a better in-IR representation and we've been skirting around this for a while. The best thing to do is to fit this into the FIRRTL spec and then figure out how to represent it here as opposed to trying to generalize don't touch on ports to don't touch on block args (as nobody is asking for that).

[youngar]

I think we should move this under if (hasUnknownSideEffect(op)). I consider this the conservative codepath for when we don't know what an operation is. Layerblocks fall under this umbrella.

If we ever had an operation like this totally made up example:

sv.ifdef 0 {
  sv.assert(clock, 0)
}

We would not want to unconditionally visit its regions.

[seldridge]

I think this seems right. It seems like the better thing would be to make this the default for any unknown operation and not just hasUnknownSideEffect. That is different from how the PR is written, though, and means that there should be early exits from all the prior if/else if clauses.

I.e., the original problem with this pass was that unless this is the fallback for all unknown operations then the pass will incorrectly delete declarations without deleting their uses.

Switched to your suggestion in: c4d490a