Skip to content
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

[Seq] Add FirRom op #5798

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

[Seq] Add FirRom op #5798

wants to merge 1 commit into from

Conversation

vowstar
Copy link

@vowstar vowstar commented Aug 8, 2023

Add a new FirRomOp to the Seq dialect to capture the
semantics of FIRRTL ROM at the HW level. The intent behind this
change is the same as for FirMemOp.

ROMs are represented as a declaration with the seq.firrom
which returns a value representing the ROM itself. This value
is of type !seq.firrom<12 x 42> which represents an opaque
handle to the ROM; it encodes the depth (12) and
width (42) of the ROM.

ROM ports are represented as separate operations:

  • seq.firrom.read_port

These operations take the ROM itself as their first operand. The
remaining operands are the specific signals required for the
corresponding type of port: clocks, enables, mode select, etc.
Read ports also return the read data as their result. Enables
are optional to allow for more concise IR for simple memory
configurations.

The reason why adding a new FirRomOp instead of reusing the
FirMemOp to process Read-Only-Memory is because there are
still huge differences in the behaviors of ROM, SRAM, and
RegFile in different foundry memory compilers, and their DFT
processing is also very different. If they are all merged into
FirMemOp, a lot of attributes must be added to
FirMemOp, resulting in the post-processing having to judge
these attributes, which increases the burden of post-processing.
Therefore, adding FirRomOp as a new op is conducive to
behavior distinction and later maintenance, especially when we
want to add DFT and physical implementation details in the real
chip design process, it will be more conducive to implementation,
and reduce the cost of code errors caused by maintenance.

This commit merely adds the FirRomOp alongside some
canonicalization and general tests, but the op isn't used by any
part of CIRCT yet. Use in FIRRTL-to-HW lowering follows as a
separate commit.

Add a new `FirRomOp` to the Seq dialect to capture the
semantics of FIRRTL ROM at the HW level. The intent behind this
change is the same as for `FirMemOp`.

ROMs are represented as a declaration with the `seq.firrom`
which returns a value representing the ROM itself. This value
is of type `!seq.firrom<12 x 42>` which represents an opaque
handle to the ROM; it encodes the `depth (12)` and
`width (42)` of the ROM.

ROM ports are represented as separate operations:

- `seq.firrom.read_port`

These operations take the ROM itself as their first operand. The
remaining operands are the specific signals required for the
corresponding type of port: clocks, enables, mode select, etc.
Read ports also return the read data as their result. Enables
are optional to allow for more concise IR for simple memory
configurations.

The reason why adding a new `FirRomOp` instead of reusing the
`FirMemOp` to process Read-Only-Memory is because there are
still huge differences in the behaviors of ROM, SRAM, and
RegFile in different foundry memory compilers, and their DFT
processing is also very different. If they are all merged into
`FirMemOp`, a lot of attributes must be added to
`FirMemOp`, resulting in the post-processing having to judge
these attributes, which increases the burden of post-processing.
Therefore, adding `FirRomOp` as a new op is conducive to
behavior distinction and later maintenance, especially when we
want to add DFT and physical implementation details in the real
chip design process, it will be more conducive to implementation,
and reduce the cost of code errors caused by maintenance.

This commit merely adds the `FirRomOp` alongside some
canonicalization and general tests, but the op isn't used by any
part of CIRCT yet. Use in FIRRTL-to-HW lowering follows as a
separate commit.

Signed-off-by: Huang Rui <[email protected]>
@seldridge
Copy link
Member

My main question with this is what is the plan on the FIRRTL Dialect side?

I'm not against the existence of a FIRRTL ROM primitive. However, I'm curious about the intent of putting this into the HW dialect directly. The rationale for adding the FIRRTL ops into seq is that users (primarily @nandor and @fabianschuiki right now) need to preserve the semantics of the FIRRTL register and memory after the LowerToHW so that they can write HW passes that work on them. The existing Seq structures (seq.compreg) and low-level SV structures (sv.always) don't properly capture the semantics of FIRRTL registers and memories and it wasn't practical to improve existing structures to create a "one register to rule them all" (for FIRRTL and any other future upstream dialects). Hence, these were added.

All that is about preserving upstream dialect semantics into downstream dialects. We do not currently have a FIRRTL ROM construct. What is the plan for how this interfaces with FIRRTL Dialect (and Chisel)? How should this be differentiated from a FIRRTL Memory with no write ports?

@sequencer
Copy link
Contributor

sequencer commented Aug 10, 2023

My main question with this is what is the plan on the FIRRTL Dialect side?

I think the ROM is another special case for Chisel Mem API(for now):
The goal of this PR is adding the ROM API like https://github.com/chipsalliance/chisel/blob/main/src/main/scala/chisel3/util/SRAM.scala

It can be constructed via Mem for now. But in the future, I think we can migrate it to an intmodule for a simple lowering path.

However, I'm curious about the intent of putting this into the HW dialect directly.

Basically I think for both SRAM and ROM are the concept which should be captured by firrtl smem ir, since they can share the same behavior model.
However they should be different IR in the HWDialect, since the circuit and metadta of which are different.

I think we may eventually get rid of the smem ir and make these two intmodule as first class intmodule for simpler lowering path and better understanding.

the plan for how this interfaces with FIRRTL Dialect (and Chisel)? How should this be differentiated from a FIRRTL Memory with no write ports?

IMHO, here is my roadmap of the ROM API:


A `seq.firrom` declares the ROM and captures the memory-level parameters
such as width and depth or how read/write collisions are resolved. The read
port are expressed as separate operations that take the declared memory as
Copy link
Contributor

Choose a reason for hiding this comment

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

This, in general is a weird representation. Operations can return multiple values. A property of a rom is how many ports it has, one shouldn't have to walk the IR to find that out.

@darthscsi
Copy link
Contributor

One known defficiency of seq.firmem is the lack of explicit representation of initialization. Once that is in, it isn't clear that a rom can't just be that op. It would help to have a more complete justification for the difference. One thing which may help is outlining how you imagine processing memories and roms in the pipeline. Essentially, where do target-specific considerations come in to the pipeline? You don't really want the compiler to be hard-coding process-specific transformations. Doing so indicates capturing the wrong thing in the IR.

@darthscsi
Copy link
Contributor

As an aside, intmodule is really not intended as a replacement for constructs which should be directly in the langauge. If a new memory or rom construct is needed to replace the (broken) current ones, then that seems pretty core to the representation.

@sequencer
Copy link
Contributor

Basically, from the view of ASIC engineer. there are three main macro macro we need to take care about. SRAM, ROM and RF. IMHO, RF and SRAM can be merged together, ROM should be a standalone implementation since the metadata, DFT are different between RAM and ROM.

As an aside, intmodule is really not intended as a replacement for constructs which should be directly in the langauge.

I think intmodule is somehow a "graybox" that has a specific behavior information and metadata other than blackbox, for ROM and RAM, that's a reasonable usage AFAIK, other "graybox" might be clock cell clkmux2, clkxor clkgate(which is already implemented).

@nandor nandor removed their request for review December 8, 2024 21:09
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