[FIRRTL] Add CheckRecursiveInstantiation diagnostic pass

[youngar]

This adds a pass to FIRRTL to check for recursive module instantiation,
which is illegal because it corresponds to infinitely sized hardware.
This uses the path based strongly connected component (SCC) algorithm
with some extra book keeping to track if there was a loop in the current
SCC or not. This is especially helpful to detect if a single-element
SCC actually contains a loop or not.

This does not use the upstream SCCIterator because we hope that this
version of the algorithm is faster by a constant factor, we don't need
to visit the SCCs in post-order, and hopefully faster handling of cycles
in single-element SCCs (which requires re-scanning all edges from a node
using the upstream version).

[youngar]

This does not support InstanceChoice, as the InstanceGraph itself does not support it yet.

[dtzSiFive]

Modules are public unless explicitly marked private, so I'm not sure what tests are intended but if these are meant to test re:private they should be updated, and if not a test or two for those cases seems warranted.

[youngar]

hmm interesting. I am guessing my code is wrong, because I am specifically checking if the visibility is public.

    // Check if the current node is public.
    if (SymbolTable::getSymbolVisibility(node->getModule<Operation *>()) ==
        SymbolTable::Visibility::Public) {
      it->getSecond() = 0;
      return 0;
    }

edit to say: code looks fine, getSymbolVisibility will return public if the attr is not there. Just need to update the test case.

[youngar]

I think this test is not working because of my depth heuristic, which is what this is meant to test, but due to the path based SCC returning nodes in pre-order. I probably need to move my top level module down to the bottom of the test case.

[youngar]

Updated the test case as mentioned, and discovered that the depth calculation was not working 😭 Hooray for tests. Anyways, pushed a fix for that, and also renamed my test files to match the name of the pass (went through a couple different names).

[youngar]

I keep thinking about it, I think my depth heuristic (specifically my implementation of it) can return the wrong results if there is an SCC instantiating another SCC. I might rework this later when I have time.

[seldridge]

This does not use the upstream SCCIterator because we hope that this
version of the algorithm is faster by a constant factor, we don't need
to visit the SCCs in post-order, and hopefully faster handling of cycles
in single-element SCCs (which requires re-scanning all edges from a node
using the upstream version).

This is worth weighing against the overhead of having another SCC implementation. If the single-element SCC is a concern, then that can be handled with a non-SCC-based pass over the modules to check for trivial self-instantiation. This could then fallback to the upstream algorithm for deeper analysis which can then assume no single-element SCCs. Alternatively, self-instantiation can be an InstanceOp verifier that checks the parent module op.

More of a thought than anything.

I weighed similar concerns with using a new SCC for layer sink and settled on the opposite finding: it was better to struggle with upstream graph traits for reasons of maintainability.

[youngar]

Ran some benchmarking, my version of the SCC was only about 25% faster, so I swapped this to use the upstream SCCIterator. As a side effect of this, this code does not find any cycles that are not reachable from the top level module.

I also removed the depth heuristic to keep the pass simple.

[dtzSiFive]

LGTM