[ModuleInliner] Donot retop a HierPathOp if flattening the root

[prithayan]

If a HierPathOp has a root module that is flattened and inlined, then donot
retop the root module, this results in incorrect HierPathOp that as stale
symbols.
This commit fixes the issue.

[dtzSiFive]

The test case is addressed by -symbol-dce, is there something here that wouldn't be handled by that (possibly + inner-sym-dce)?

[prithayan]

The test case is addressed by -symbol-dce, is there something here that wouldn't be handled by that (possibly + inner-sym-dce)?

No, this is expected to do what the symbol-dce would do. But, do we want to run the dce before module-inliner.
My thought was that, if an unused HierPathOp is legal in the IR, then the module-inliner must be able to handle it independently, instead of crashing or being dependent on the dce pass.

[dtzSiFive]

Thanks, yes this should not turn legal IR into illegal IR 👍 .

[uenoku]

Thank you for investigating the issue and the fix makes sense to me:) I’m a bit concerned about partially duplicating symbol dce logic into inliner. Liveness of symbols is not solely determined by annotations users so this implementation could incorrectly erase hier path ops that are used by other operations such as XMRRef or Verbatim (though I think it won’t happen in the current pass ordering). Instead can we fix the issue by properly updating every hierpath in the IR, or tentatively regard symbol dce as a prepass of inliner? WDYT?

[prithayan]

Thank you for investigating the issue and the fix makes sense to me:) I’m a bit concerned about partially duplicating symbol dce logic into inliner. Liveness of symbols is not solely determined by annotations users so this implementation could incorrectly erase hier path ops that are used by other operations such as XMRRef or Verbatim (though I think it won’t happen in the current pass ordering). Instead can we fix the issue by properly updating every hierpath in the IR, or tentatively regard symbol dce as a prepass of inliner? WDYT?

I agree, the correct solution here is to update the inlining algorithm to not rely on the circt.nonlocal annotations, I will try to fix the algorithm, instead of the duplicate dce logic.

[dtzSiFive]

cc #4462

[prithayan]

After spending hours staring at the code, I realized this is not the issue of removing dead NLAs, this issue exists even if the NLA is referenced from a circt.nonlocal. Seems like a simple fix. I will do some more testing and merge the fix.