Skip to content

rtlil: enable single-bit vector wires #5095

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 3 commits into from
May 23, 2025
Merged

rtlil: enable single-bit vector wires #5095

merged 3 commits into from
May 23, 2025

Conversation

widlarizer
Copy link
Collaborator

@widlarizer widlarizer commented May 6, 2025

In some tools, there is a distinction between the following two wire shapes:

wire foo;
wire [0:0] bar;

In this PR, I enable yosys to distinguish between them by reusing RTLIL::Wire::upto as RTLIL::Wire::sbvector (single-bit vector) in a safe-to-transmute union as the "upto" field doesn't carry meaning for single-bit wires. There's a corresponding AstNode::is_sbvector. This change doesn't affect synthesis and retains the distinction for the sake of frontends and backends. Namely, RTLIL, verilog, and json can carry this information at the moment. Verilog distinguishes them as shown above, RTLIL uses explicit width 1, and json has a new sbvector field for ports and nets.

@widlarizer widlarizer requested a review from zachjs as a code owner May 6, 2025 11:24
@widlarizer widlarizer requested review from jix and removed request for zachjs May 6, 2025 11:28
Copy link
Member

@whitequark whitequark left a comment

Choose a reason for hiding this comment

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

I think this complicates RTLIL further without any clear (or stated justification was added after I posted the comment) benefit, and causes churn to RTLIL consumers.

@widlarizer
Copy link
Collaborator Author

Yeah I accidentally sent an unwritten description at first. Anyway, the distinction somehow does exist in the domain yosys is modeling (systemverilog). The design is with the full intention of the impact on downstream users being minimal as it's possible to entirely ignore this change without functional damage or build breakage

also cc @QuantamHD @mikesinouye

@whitequark
Copy link
Member

Anyway, the distinction somehow does exist in the domain yosys is modeling (systemverilog).

I was under impression that Yosys supports multiple languages (SV, VHDL, Amaranth, etc) rather than being beholden to being a SystemVerilog frontend. If it was the case that it is modeling SystemVerilog then it does a remarkably awful job of it since it doesn't preserve types, interfaces, enumerations, logic vs wire, does a haphazard job representing strings, and so on. Singling out a 1-bit vector change to be added as a core RTLIL concept rather than, say, an attribute is pretty odd given this context.

@whitequark
Copy link
Member

whitequark commented May 6, 2025

I think this should be an attribute, similar to how SV enumerations are modelled using an attribute. Making it an attribute will reduce impact on downstream users since attributes will already roundtrip through any existing RTLIL or Yosys JSON consumer (including old versions of Yosys) while sbvector will not.

@povik
Copy link
Member

povik commented May 6, 2025

I agree with Catherine's assessment and I'm also curious: What's the concrete use case where preserving this distinction is critical?

@mikesinouye
Copy link
Contributor

I agree with Catherine's assessment and I'm also curious: What's the concrete use case where preserving this distinction is critical?

We'd like to get the output netlist to have consistent naming conventions with the default behavior of other downstream tools. As an example, after ingesting the Yosys netlist into openroad later we'd like compatibility with already-written SDCs, floorplans, UPF, etc. generated by other tools.

In a Yosys flow, after running splitnets -ports, wires with [0:0] will be handled differently than single-bit wires. However, other tools preserve the array, so when they run their equivalent splitnets -ports pass, things are named according to array conventions causing mismatches as these wires were implictly converted to single bits in Yosys.

As added constraints, we are unsure which signals will be defined as [0:0] beforehand as these are typically parameterized, and we cannot edit the design or downstream artifacts.

In general we are happy with any approach to get this desired behavior, an alternative method to support this use case seems fine by us as well, but this PR's approach solves our issue.

@jix
Copy link
Member

jix commented May 7, 2025

My reasoning for keeping this in Wire was that I consider this to be part of the same data as offset and I wanted to keep both at the same place. (I do think it was a mistake to have offset part of Wire given that it doesn't affect RTLIL semantics at all and is just there to pass through information from Verilog and VHDL.)

I thought we might be able to do that without causing any backwards compatibility issues but the a) current PR makes some decisions that change things beyond what I was originally thinking of and b) I forgot to consider external producers of textual RTLIL, which I don't want to break.

With that, I do think using an attribute here would be the best choice for now. I also think we can afford the characters to call it single_bit_vector so it's easier for users to figure out what it means.

@widlarizer widlarizer force-pushed the emil/one-bit-width branch from a4d23c0 to e5171d6 Compare May 12, 2025 11:24
Copy link
Member

@whitequark whitequark left a comment

Choose a reason for hiding this comment

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

LGTM

@mikesinouye
Copy link
Contributor

Great! Thank you for working on this. Is this ready to be merged?

@widlarizer widlarizer merged commit 4b8d42d into main May 23, 2025
43 checks passed
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.

5 participants