[FIRRTL] Remove "ref foo is <string>" syntax

[seldridge]

Now that probes have been operational for some time, we no longer need to accept non-standard formats that allow string paths to be exposed from inside external modules. This has been fully subsumed with the FIRRTL ABI that produces a ref file of Verilog macros.

E.g., there are tests that accept the following in parse-basic.fir:

  extmodule RefExtMore :
    input in : UInt<1>
    output r : Probe<UInt<1>>
    output data : UInt<3>
    output r2 : Probe<{a : UInt<3>}[3]>
    ref r2 is "in"
    ref r is "path.to.internal.signal"

These tests, parsing, and any handling of "internalpaths" can be removed.

[dtzSiFive]

These are and have been in the FIRRTL spec since probes/ref types were introduced.

You could think of them as ref literals, and pre-date the ABI.

They may have utility for use with non-FIRRTL-defined modules, e.g. https://github.com/chipsalliance/chisel/pull/3486 .

Having Chisel emit "ABI" files directly itself for this use case is a pain. And arguably the wrong level but makes it not part of the declaration which has strengths.

Perhaps a friction that some tooling could assist or just a burden to take on anyway (as that linked PR did for an earlier ABI, sorry if that broke with the ABI change by the way!)?

Anyway they're not currently used but they're not non-standard so uh please do not remove them until they are.

They're also the only way to XMR into/through an intrinsic, which is ... fun 🙃.. but was requested when I touched this last, so a consideration for this direction.

On Sat, Feb 17, 2024, 10:21 PM Schuyler Eldridge @.***> wrote:

Now that probes have been operational for some time, we no longer need to accept non-standard formats that allow string paths to be exposed from inside external modules. This has been fully subsumed with the FIRRTL ABI that produces a ref file of Verilog macros.

E.g., there are tests that accept the following in parse-basic.fir:

extmodule RefExtMore : input in : UInt<1> output r : Probe<UInt<1>> output data : UInt<3> output r2 : Probe<{a : UInt<3>}[3]> ref r2 is "in" ref r is "path.to.internal.signal"

These tests, parsing, and any handling of "internalpaths" can be removed.

— Reply to this email directly, view it on GitHub https://github.com/llvm/circt/issues/6715, or unsubscribe https://github.com/notifications/unsubscribe-auth/AYZWN5LACBGBVCNASFIV3FLYUF6VBAVCNFSM6AAAAABDN22J3KVHI2DSMVQWIX3LMV43ASLTON2WKOZSGE2DANZSHA4DENI . You are receiving this because you are subscribed to this thread.Message ID: @.***>

[seldridge]

Having Chisel emit "ABI" files directly itself for this use case is a pain.

I actually think this is the correct approach. It is always allowable for a user to work with the ABI directly. We should encourage this.

Thinking about it differently... Chisel, if being used to printf Verilog directly shouldn't be any more privileged than somebody writing that Verilog by hand. If this was integration of the latter, would we advise use of the ABI?

not currently used...

Seems like a safe time to roll them back. 😉

Probe an intrinsic...

Do we have a use case for this?

[dtzSiFive]

need to accept non-standard formats

Fix?

---

As for the rest, wrote from phone so perhaps some tone was lost -- "here are other things you might also not be aware of".

Having a stable versioned ABI for these things sounds good, but still presently it's a bit painful to roll code to emit the right macros for. That was not mean to say ref statements are better or even good. And "wrong layer" I mean that it encodes what's known at a high-level directly into low-level implementation details, making it impossible to understand this information without understanding the verilog macros. "Wrong" is maybe hasty but hopefully you see what I mean.

This means FIRRTL itself cannot be used to represent a circuit's probes into non-FIRRTL bits, without some sort of Verilog preprocessor + file scan or whatever, and this may be undesirable if wanting the extmodule to be a simulated thing or whatever.

Practically not a big loss but there's a more complete explanation. This is in some small part also suggesting maybe having cross-module information encoded as Verilog macros has some downsides, said another way?

Your reasoning about wanting the ABI used makes sense, given that ship has sailed. Thanks.

Anyway as mentioned, the pain can be helped by maintaining Chisel code to understand and emit the FIRRTL module ABI -- or other tooling that helps do this when interacting with non-Chisel-generated modules.

---

Dropping sounds good :+1: .

One fewer thing to worry about and maintain / forget about and have interact poorly when someone tries to use it.