Skip to content

[FIRRTL][LowerXMR] Add inner symbols on ports#9954

Open
seldridge wants to merge 1 commit intomainfrom
dev/seldridge/firrtl-lower-xmr-read-probes-on-ports
Open

[FIRRTL][LowerXMR] Add inner symbols on ports#9954
seldridge wants to merge 1 commit intomainfrom
dev/seldridge/firrtl-lower-xmr-read-probes-on-ports

Conversation

@seldridge
Copy link
Member

When a RefSendOp references a port (block argument), add an inner symbol
directly in the LowerXMR pass instead of creating an intermediate node.
This produces cleaner output and avoids unnecessary intermediate wires.
This is additionally done to work around problems with certain formal
tools which treat output ports that are assigned the value of a probe node
as "uncovered".

Before:

firrtl.module @Foo(in %a: !firrtl.uint<8>) {
  %a_probe = firrtl.node sym @sym interesting_name %a
}
// Verilog: wire [7:0] a_probe = a;
// XMR: `define ref_Foo_a_probe a_probe

After:

firrtl.module @Foo(in %a: !firrtl.uint<8> sym @sym) {
}
// Verilog: (no intermediate wire)
// XMR: `define ref_Foo_a a

AI-assisted-by: Augment (Claude Sonnet 4.5)

When a `RefSendOp` references a port (block argument), add an inner symbol
directly in the `LowerXMR` pass instead of creating an intermediate node.
This produces cleaner output and avoids unnecessary intermediate wires.
This is additionally done to work around problems with certain formal
tools which treat output ports that are assigned the value of a probe node
as "uncovered".

Before:

    firrtl.module @foo(in %a: !firrtl.uint<8>) {
      %a_probe = firrtl.node sym @sym interesting_name %a
    }
    // Verilog: wire [7:0] a_probe = a;
    // XMR: `define ref_Foo_a_probe a_probe

After:

    firrtl.module @foo(in %a: !firrtl.uint<8> sym @sym) {
    }
    // Verilog: (no intermediate wire)
    // XMR: `define ref_Foo_a a

AI-assisted-by: Augment (Claude Sonnet 4.5)
Signed-off-by: Schuyler Eldridge <schuyler.eldridge@sifive.com>
@seldridge seldridge requested a review from darthscsi as a code owner March 16, 2026 17:09
@seldridge seldridge requested a review from dtzSiFive March 16, 2026 17:09
Copy link
Contributor

@dtzSiFive dtzSiFive left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM.

Thank you for your care about the various situations.
They should (have been) under in-tree testing.

This aligns with your other change on preferring nicer output at cost of losing (unrealized) optimization opportunity, which makes sense to me that these work together.

As discussed (just noting here) formal use cases are still fragile if they rely on specifically what declaration the probe XMR ends up on and this IIRC wont work for them for output ports that are read from in FIRRTL.

One day we'll support both optimization opportunity and clean output. Thanks for working on these!

I have not carefully reviewed tests, apologies.

Just curious, how is this handling indexing operations? Specifically indexing of ports. In case aggregates are preserved?

Doing a little getFieldRefFromValue and using field-specific inner symbol might be nice?

I'm not 100% if we spill it if we don't, guessing we would since node with symbol on it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants