[firtool] fix: check uninferred resets after removing unused modules

[unlsycn]

The Chisel's Definition API generates modules that are not instantiated, whose reset cannot be inferred properly. Remove them before InferResets pass to resolve it.

See chisel#4292

[unlsycn]

cc @nandor @jackkoenig

[dobios]

Nice catch, would it be possible to add a test that shows that moving this actually solves the problem in question?

[jackkoenig]

It's really important that reset inference runs before dedup, so if this general approach is used to solve the problem, it needs to be that empty modules are removed earlier rather than infer resets running later.

[unlsycn]

It's really important that reset inference runs before dedup, so if this general approach is used to solve the problem, it needs to be that empty modules are removed earlier rather than infer resets running later.

I see. So what if I only defer the abstract reset check rather than reset inference after removing unreferenced modules? If that's acceptable, do you think it's better to do this in Inliner or create a new pass to do it?

Another way is to run the checks in the according passes, but emit the error after running Inliner, which the problem is how to pass the Diagnostic to the later passes.

Approach looks reasonable to me but I will defer to the maintainers on reviewing it!

[jackkoenig]

I think this looks reasonable but will defer to folks like @fabianschuiki on if it should be merged.

Having done some recent work on InferResets, I'm thinking more and more that we should reframe reset inference as a simpler propagation that occurs eagerly in Chisel, but that will cause some issues for rocket-chip's Diplomacy that I need to think through first. That would also solve this issue, but I think it will be a while before there's any movement there.

[dobios]

Thanks for iterating on this! I see the benefit of splitting this off into a separate pass, however I do have some comments on various pass details.

[dobios]

Please also add a test that demonstrates the correctness of your new pass and not just the firrtl lowering as a whole.

[unlsycn]

The test of uninferred reset check has been included in infer-resets-errors.mlir, the test which we actually need to add is to compare the errors emitted with or without enabling firrtl-inliner.

[unlsycn]

I'm tend to preserve this test using firtool since it is extracted from an actual use case, while adding a new minimal test. What do you think.

[seldridge]

The main problem with this is that it is relying on the inliner to remove modules which are not instantiated. The inliner doesn't have to remove these modules. Or: this is relying on an optimization being required for correctness. (The inliner is removing dead modules because it is trying to clean up after itself after it inlines modules. This happens to cull all uninstantiated modules even if they weren't made uninstantiated by the inliner. It would be better if this was a separate pass and not handled in the inliner.)

The question is then what should happen in the FIRRTL specification for the following two situations:

1. A private module is uninstantiated with an abstract reset.
2. A public module is uninstantiated with an abstract reset.

For public modules, this has to treat them all as independent tops and infer accordingly. For private modules, it's maybe less clear, but the correct behavior is probably to treat them as separate tops as well. I expect the root of the problem is just that InferResets is relying on an assumption of a single top module or that this is using FIRRTL's instance graph which doesn't understand multiple roots like HW's instance graph does.

[unlsycn]

The main problem with this is that it is relying on the inliner to remove modules which are not instantiated. The inliner doesn't have to remove these modules. Or: this is relying on an optimization being required for correctness. (The inliner is removing dead modules because it is trying to clean up after itself after it inlines modules. This happens to cull all uninstantiated modules even if they weren't made uninstantiated by the inliner. It would be better if this was a separate pass and not handled in the inliner.)

The question is then what should happen in the FIRRTL specification for the following two situations:

1. A private module is uninstantiated with an abstract reset.
2. A public module is uninstantiated with an abstract reset.

For public modules, this has to treat them all as independent tops and infer accordingly. For private modules, it's maybe less clear, but the correct behavior is probably to treat them as separate tops as well. I expect the root of the problem is just that InferResets is relying on an assumption of a single top module or that this is using FIRRTL's instance graph which doesn't understand multiple roots like HW's instance graph does.

Thanks for your explanation! I have considered extracting the cleanup from the inliner, but that should introduce an extra top-down process that would affect performance. I'll give a try now.

[dtzSiFive]

Can Chisel not emit modules that are unused, or what's the context here? That seems useful anyway (if nothing else it seems wasteful especially if taken too far-- "generate definitions for all parameterizations JIC") if it can be done neatly.

Anyway assuming that it's necessary (or good?) for Chisel to emit unused modules:

Let's be careful about this and make sure whatever is done is part of a consistent larger story and not a one-off solution to a specific issue. There should be a reasoning justifying the change, otherwise the system becomes fragile and small ordering and behavioral details are load-bearing (but no one quite knows which or why anymore...) and makes it very difficult to alter or implement other tools working with FIRRTL as they'll have to discover and re-implement all these details.

In particular, regarding this issue and direction -- I am uncertain about hiding errors in uninstantiated modules by removing things that would be an error if instantiated, which this seems a small step towards. If an uninstantiated private module has uninferred widths...? Or undriven signals (including or not input signals), comb cycles, or falls in the forest and no one is around... is it an error?
CIRCT is presently careful to avoid optimizing (instantiated) design modules much until these sorts of things have been identified and resolved, it's unclear just because these are trivially dead that's any different. Dead hardware (e.g., wires) that are undriven are presently errors, why is that different?

This seems like something to sort out in the FIRRTL specification and make the compiler treat uniformly and not reliant on the behavior or ordering of optimizations. Probably with a little thought this can be quickly sorted out.

Seemingly well-formed modules that are uninstantiated don't seem unreasonable to allow (someone mentioned the spec might have previously disallowed dead modules but I don't see that in a quick bounce through), but let's pin down the specifics please.

The question is then what should happen in the FIRRTL specification for the following two situations:

1. A private module is uninstantiated with an abstract reset.
2. A public module is uninstantiated with an abstract reset.

Thanks, well put. And you're right in the presence of multiple tops nothing works presently. We should prioritize this and see that through soon.

FWIW public modules cannot have abstract resets on their ports, so their instantiation should not impact whether abstract resets can be inferred, but we should visit it to infer its contents/children anyway.

For private modules, that's interesting. Since they are not instantiated and cannot be (reliably), any abstract reset input ports will be unconstrained. Today that would be an error, even if they're visited.

---

The main problem with this is that it is relying on the inliner to remove modules which are not instantiated. The inliner doesn't have to remove these modules. Or: this is relying on an optimization being required for correctness. (The inliner is removing dead modules because it is trying to clean up after itself after it inlines modules. This happens to cull all uninstantiated modules even if they weren't made uninstantiated by the inliner. It would be better if this was a separate pass and not handled in the inliner.)

👍

We do have IMDCE, for example, which will do this, of course. It is not suited for running at that point in the pipeline for multiple reasons. Adding a special pass that prunes trivially dead modules could be a thing but the game should not be "what can we implement that works right now" -- just because we can doesn't mean we should.

I'm sure there are concrete and valuable needs driving this, all the more reason to ensure we have a solid foundation.

[unlsycn]

I see. Thanks for the reminder.

Can Chisel not emit modules that are unused, or what's the context here? That seems useful anyway (if nothing else it seems wasteful especially if taken too far-- "generate definitions for all parameterizations JIC") if it can be done neatly.

It seems to be more complicated to remove uninstantiated modules in Chisel rather than in CIRCT. And I don't think FIRRTL with dead modules should be considered an illegal input.

In particular, regarding this issue and direction -- I am uncertain about hiding errors in uninstantiated modules by removing things that would be an error if instantiated, which this seems a small step towards. If an uninstantiated private module has uninferred widths...? Or undriven signals (including or not input signals), comb cycles, or falls in the forest and no one is around... is it an error?
CIRCT is presently careful to avoid optimizing (instantiated) design modules much until these sorts of things have been identified and resolved, it's unclear just because these are trivially dead that's any different. Dead hardware (e.g., wires) that are undriven are presently errors, why is that different?

The difference is the undriven signals are the errors in the module definition and the uninferred resets are about instances (context-sensitive), as how a module with an abstract reset is inferred depends on the module in which it is instantiated.