[FIRRTL][IMDCE] Add test for dead output ports, NFC #5512
Conversation
This adds a test to show the behaviour that when a dead module instantiates a child module, and uses its output port, that the dead module should not demand the child's port and keep it alive. Technically FIRRTL does not allow dead modules in the circuit, but this pass already supports this.
|
This is a follow up from #5480. |
| // This module is dead. Since it is the only user of @Blah's output port, the | ||
| // output should be deleted. | ||
| firrtl.module private @Other(out %out : !firrtl.uint<1>) { | ||
| // CHECK: %0 = builtin.unrealized_conversion_cast to !firrtl.uint<1> |
There was a problem hiding this comment.
Ah, I didn't imagine there was a corner case where unrealized_conversin cast outlives IMDCE pass. Probably we have to just erase modules unreachable from toplevel.
There was a problem hiding this comment.
Yeah. The FIRRTL Inliner will delete unreachable modules before this pass runs, so this isn't reachable in the firtool pipeline. Arguably the inliner should not be deleting modules at all, but at the time it was trying to maintain the invariant that all modules are reachable from the top level, and so it was supposed to be deleting the modules where every instance was inlined.
There was a problem hiding this comment.
at the time it was trying to maintain the invariant that all modules are reachable from the top level, and so it was supposed to be deleting the modules where every instance was inlined.
That's totally reasonable and I think that's good thing to delete such modules in the early pipeline 👍 #5517 should clean up unreachable modules in IMDCE as well.
As pointed out in #5512, IMDCE could leave unrealized conversion casts which are intended only for dummy values if there was an unreachable module from toplevel. Even though it won't practically happen, it would be reasonable to perform such DCE in IMDCE.
As pointed out in #5512, if there was an unreachable module from the toplevel, IMDCE could leave unrealized conversion casts which are intended only for dummy values. Even though it won't practically happen, it would be reasonable to perform such DCE in IMDCE.
This adds a test to show the behaviour when a dead module instantiates a child module, and uses its output port, that the dead module should not demand the child's port and keep it alive. Technically FIRRTL does not allow dead modules in the circuit, but this pass already supports this.
As pointed out in llvm#5512, if there was an unreachable module from the toplevel, IMDCE could leave unrealized conversion casts which are intended only for dummy values. Even though it won't practically happen, it would be reasonable to perform such DCE in IMDCE.
This adds a test to show the behaviour when a dead module instantiates a child module, and uses its output port, that the dead module should not demand the child's port and keep it alive. Technically FIRRTL does not allow dead modules in the circuit, but this pass already supports this.