[Arc] Handle sv.xmr.ref ops and hw.hierpath ops

[nanjo712]

The LowerState pass flattens the module hierarchy and removes instances, but it leaves hw.hierpath operations intact. These paths maintain symbol references to hw.module.extern operations. When the pass later attempts to strip these external modules, the symbol table check fails with the error: "Failed to remove external module because it is still referenced/instantiated".

Since Arcilator targets a flattened simulation model, hierarchical paths are no longer valid or necessary after the modules are lowered. I adds a cleanup step to erase all hw::HierPathOp operations within LowerStatePass before performing the final external module removal.

[fabianschuiki]

Hey @nanjo712, thanks for adding this! One question: could we do this earlier in the pipeline, for example in StripSV? It feels weird that LowerState would do hw.hier_path op cleanup as a side effect, since it's otherwise only concerned about converting hw.modules into arc.models.

[nanjo712]

Hi @fabianschuiki ,Thank you for your review.

I agree that doing cleanup of hw.hierpath in LowerState does seem odd. For the current implementation, it feels more like a temporary patch. However, I also noticed another issue in my work yesterday: simply deleting all hw.hierpaths might not be appropriate.

From what I've observed, a performance counter-like structure generates sv.xmr.ref, creating references to symbols defined in hw.hierpath.

Based on this, we might need to distinguish between references to internal paths and references to external paths? Is it possible to introduce a new transformation at an earlier stage to handle sv.xmr.ref or any references to hierpath? My initial thought is that hierpath already provides enough information that we should be able to convert these symbols into references to actual signals?

[nanjo712]

I've added a new pass specifically for handling sv.xmr.ref and hw.hierpath, exposing the internal signals upwards via recursive hole punching, and then directly converting sv.xmr.ref to a reference to the signal.

I'd love to hear your feedback on this.

[fabianschuiki]

I do love the idea of materializing sv.xmr.ref back to proper signal references. It's a shame that the FIRRTL pipeline doesn't do this on its own, but that would be a more difficult fix. Does this work for all weird ways in which XMR references are used? Like reading from remote signals, assigning to them, resolving multi-driver issues and all that? Do we need to error out on certain inputs instead of silently miscompiling inputs? For example, should we analyse whether an XMR ref is used for reading or for writing, and then create an actual input or output port that carries the signal, instead of an !hw.inout? I'm not 100% sure how we handle !hw.inout at the moment in Arcilator -- if at all.

[nanjo712]

Thank you for your review. I agree with your concerns.

I've recently been trying to simulate a large-scale processor designed using Chisel using Arcilator. During compilation, I noticed that Arcilator currently doesn't handle sv.xmr.ref and hw.hierpath, which prevents downgrading to LLVM IR. Based on my current observations, Chisel currently generates sv.xmr.ref references of type hw.inout, and then uses sv.read_inout to read signals. This is the main reason for my current design.

In the current version, I've narrowed the semantic scope to avoid some silent compilation errors.

For the current version, we only support read-only XMR usage; any non-read-only usage will result in an error. Furthermore, internal signal references to black-box modules will result in an error by default; the downgrading to 0 strategy will only be enabled by enabling the option. Tests for these error paths have been added, and I've also fixed some previously uncovered path reuse bugs.

In the long term, supporting more XMR usages should be necessary. However, I believe that for now, supporting read-only paths should be sufficient to cover most cross-module connection needs.

If there is a need for support for more operations such as write paths, I think it might be more appropriate to submit a new patch?

Looking forward to your reply.