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.

Sign up for GitHub

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

Jump to bottom

[FIRRTLToHW] Lower memories to seq.firmem #5025

Merged
merged 1 commit into from
Jul 11, 2023
Merged

[FIRRTLToHW] Lower memories to seq.firmem #5025

merged 1 commit into from
Jul 11, 2023

Conversation

fabianschuiki
Copy link
Contributor

Make LowerToHW emit seq.firmem operations for memories instead of emitting generator ops. These memory ops are then lowered to generator ops later in the firtool pipeline through the Seq LowerFirMem pass, which is already in place.

The main change is replacing the generator op creation in LowerToHW with code that constructs the FirMem and associated ports, which is conceptually simpler.

This commit also outlines memory lowering tests into a separate lower-to-hw-memories.mlir file and combines it with a few more test cases.

The only change to firtool Verilog output should be a loss of the _combMem suffix on memories.

Follow-up work: Once this lands, seq.firmem and seq.hlmem can be collapsed into a unified seq.mem. Passes like HWMemSimImpl (and at least one SiFive-internal pass I can think of) can then be updated to work with seq.mem directly. Afterwards the seq.mem-to-hw.module.generated lowering pass can be removed again.

@fabianschuiki fabianschuiki added FIRRTL Involving the `firrtl` dialect Seq Involving the `seq` dialect labels Apr 12, 2023
@fabianschuiki fabianschuiki force-pushed the fschuiki/firrtl-lower-to-seq-firmem branch 2 times, most recently from 0cfe049 to 48ba4fd Compare April 12, 2023 22:19
@fabianschuiki fabianschuiki force-pushed the fschuiki/firrtl-lower-to-seq-firmem branch from 48ba4fd to 594ac4c Compare April 12, 2023 22:51
@fabianschuiki fabianschuiki force-pushed the fschuiki/firrtl-lower-to-seq-firmem branch from 594ac4c to 3fc40f0 Compare April 13, 2023 04:13
@fabianschuiki fabianschuiki force-pushed the fschuiki/firrtl-lower-to-seq-firmem branch from 3fc40f0 to 62cc0b7 Compare April 28, 2023 21:09
@fabianschuiki fabianschuiki force-pushed the fschuiki/firrtl-lower-to-seq-firmem branch from 62cc0b7 to fb9a3d3 Compare June 22, 2023 16:36
@fabianschuiki fabianschuiki force-pushed the fschuiki/firrtl-lower-to-seq-firmem branch 2 times, most recently from 513dc4e to 563f1f0 Compare June 22, 2023 17:23
@mortbopet mortbopet mentioned this pull request Jun 23, 2023
Open
@fabianschuiki fabianschuiki force-pushed the fschuiki/firrtl-lower-to-seq-firmem branch from 563f1f0 to eecc11e Compare June 26, 2023 17:06
Base automatically changed from fschuiki/lower-firmem to main June 26, 2023 17:34
@fabianschuiki fabianschuiki force-pushed the fschuiki/firrtl-lower-to-seq-firmem branch from eecc11e to 1354a99 Compare June 26, 2023 17:36
@fabianschuiki fabianschuiki force-pushed the fschuiki/firrtl-lower-to-seq-firmem branch from 1354a99 to 30c580e Compare July 5, 2023 19:19
uenoku
uenoku reviewed Jul 11, 2023
View reviewed changes
test/firtool/chirrtl.fir
input clock: Clock
input data: UInt<8>
output out0: UInt<8>
output out1: UInt<8>

; CHECK: testmem_combMem testmem_ext (
Copy link
Member

Choose a reason for hiding this comment

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

Dropping the suffix combMem is reasonable, but maybe internally there is a hacky logic that detects combinatorial memories by checking the suffix. I think it's totally OK to drop the suffix at will but it would be nice to check beforhand. It seems the suffix is introduced by 2167a06, @prithayan do you remember if that was requirement?

Copy link
Contributor

Choose a reason for hiding this comment

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

Makes sense to drop the combMem, I don't think any tool is relying on that.

uenoku
uenoku approved these changes Jul 11, 2023
View reviewed changes
Copy link
Member

@uenoku uenoku left a comment

Choose a reason for hiding this comment

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

Dropping suffix may cause some problems but otherwise LGTM!

@fabianschuiki fabianschuiki force-pushed the fschuiki/firrtl-lower-to-seq-firmem branch from 30c580e to eba7c86 Compare July 11, 2023 17:45
@fabianschuiki
Copy link
Contributor Author

Thanks! I'll run another sanity check on some internal design.

@fabianschuiki
[FIRRTLToHW] Lower memories to seq.firmem
f21371a 
Make `LowerToHW` emit `seq.firmem` operations for memories instead of
emitting generator ops. These memory ops are then lowered to generator
ops later in the firtool pipeline through the Seq `LowerFirMem` pass,
which is already in place.

The main change is replacing the generator op creation in `LowerToHW`
with code that constructs the FirMem and associated ports, which is
conceptually simpler.

This commit also outlines memory lowering tests into a separate
`lower-to-hw-memories.mlir` file and combines it with a few more test
cases.

The only change to firtool Verilog output should be a loss of the
`_combMem` suffix on memories.
@fabianschuiki fabianschuiki force-pushed the fschuiki/firrtl-lower-to-seq-firmem branch from eba7c86 to f21371a Compare July 11, 2023 22:22
@fabianschuiki
Copy link
Contributor Author

Checked a few of our internal designs and they still work with the change. Merging this in. In case of unexpected breakages we'll back this out again.

@fabianschuiki fabianschuiki merged commit 0f5c714 into main Jul 11, 2023
5 checks passed
@fabianschuiki fabianschuiki deleted the fschuiki/firrtl-lower-to-seq-firmem branch July 12, 2023 16:11
calebmkim pushed a commit to andrewb1999/circt that referenced this pull request Jul 12, 2023
@fabianschuiki @calebmkim
[FIRRTLToHW] Lower memories to seq.firmem (llvm#5025)
a160fc2 
Make `LowerToHW` emit `seq.firmem` operations for memories instead of
emitting generator ops. These memory ops are then lowered to generator
ops later in the firtool pipeline through the Seq `LowerFirMem` pass,
which is already in place.

The main change is replacing the generator op creation in `LowerToHW`
with code that constructs the FirMem and associated ports, which is
conceptually simpler.

This commit also outlines memory lowering tests into a separate
`lower-to-hw-memories.mlir` file and combines it with a few more test
cases.

The only change to firtool Verilog output should be a loss of the
`_combMem` suffix on memories, alongside name changes.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@uenoku uenoku uenoku approved these changes

@seldridge seldridge Awaiting requested review from seldridge seldridge is a code owner

@rwy7 rwy7 Awaiting requested review from rwy7

@prithayan prithayan Awaiting requested review from prithayan

@dtzSiFive dtzSiFive Awaiting requested review from dtzSiFive

@darthscsi darthscsi Awaiting requested review from darthscsi darthscsi is a code owner

Assignees
No one assigned
Labels
FIRRTL Involving the `firrtl` dialect Seq Involving the `seq` dialect
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@fabianschuiki @prithayan @uenoku