Skip to content

[FIRRTLToHW] Add support for lower-to-core in firrtl.fflush#10545

Open
nanjo712 wants to merge 6 commits into
llvm:mainfrom
nanjo712:feat/flush-in-lower-to-core
Open

[FIRRTLToHW] Add support for lower-to-core in firrtl.fflush#10545
nanjo712 wants to merge 6 commits into
llvm:mainfrom
nanjo712:feat/flush-in-lower-to-core

Conversation

@nanjo712
Copy link
Copy Markdown
Contributor

@nanjo712 nanjo712 commented May 26, 2026

Add support for lower-to-core in firrtl.fflush

Part of #10131

@nanjo712 nanjo712 force-pushed the feat/flush-in-lower-to-core branch from 9182b5d to 922f713 Compare May 26, 2026 14:01
@circt-bot
Copy link
Copy Markdown

circt-bot Bot commented May 26, 2026

Results of circt-tests run for 922f713 compared to results for 7d284c1: no change to test results.

Comment thread test/firtool/lower-to-core.fir Outdated
; CHECK-NEXT: [[FILE2:%.+]] = sim.get_file [[FMTFILE2]]
; CHECK-NEXT: sim.proc.print [[MSG]] to [[FILE2]]
; CHECK-NEXT: }
; CHECK: sim.triggered %clock if %enable {
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I know it's a hard problem but is there a way/plan to merge fprintf/fflush in the same sim.triggered statement? Or are we going to introduce a token type to provide order in graph region?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

We have a SquashSimTriggered pass in the Sim dialect to merge sim.triggered operations with the same clock, while sinking the condition to an internal scf.if. For now, I believe this should be sufficient to maintain the current behavior of FIRRTL to SV.

@nanjo712
Copy link
Copy Markdown
Contributor Author

Regarding the need to introduce tokens to provide order semantics in graph regions, my personal understanding is that simply providing order semantics in the downstream dialect might not be very meaningful. If we need a sufficiently strong order guarantee, I think we would need to make some modifications at least at the FIRRTL level.

Based on my own observations, I think introducing a process block in FIRRTL would be a simpler approach, and tokens are also a common practice. At this point, I'm not entirely sure which approach we would prefer.

@uenoku
Copy link
Copy Markdown
Member

uenoku commented May 27, 2026

I think introducing a process block in FIRRTL would be a simpler approach

FIRRTL uses SSACFG, so it's ordered arguably. However in Sim dialect the order of sim.triggered is not defined.

@nanjo712
Copy link
Copy Markdown
Contributor Author

FIRRTL uses SSACFG, so it's ordered arguably.

Sorry for my misunderstanding, that makes sense.

However in Sim dialect the order of sim.triggered is not defined.

Can we execute SquashSimTriggered as early as possible after FIRRTLToHW to maintain the order as much as possible? This is also the main purpose of introducing this pass.

@nanjo712 nanjo712 force-pushed the feat/flush-in-lower-to-core branch from 922f713 to 8fbe8bf Compare May 27, 2026 09:21
@nanjo712
Copy link
Copy Markdown
Contributor Author

In the firtool pipeline, I added a Squash pass after LowerToHW. Currently, this behavior seems to be as expected?

@circt-bot
Copy link
Copy Markdown

circt-bot Bot commented May 27, 2026

Results of circt-tests run for 8fbe8bf compared to results for e2cbd2d: no change to test results.

@nanjo712 nanjo712 requested a review from uenoku May 27, 2026 09:57
@nanjo712 nanjo712 force-pushed the feat/flush-in-lower-to-core branch from 8fbe8bf to cec5065 Compare May 27, 2026 10:08
@circt-bot
Copy link
Copy Markdown

circt-bot Bot commented May 27, 2026

Results of circt-tests run for cec5065 compared to results for e2cbd2d: no change to test results.

@seldridge
Copy link
Copy Markdown
Member

In the firtool pipeline, I added a Squash pass after LowerToHW. Currently, this behavior seems to be as expected?

It seems better if this was handled as part of the LowerToHW pass pipeline as opposed to after, while I recognize that it can work either way. My reasoning here is that after LowerToHW, it's good if any expectations about the FIRRTL are enforced as opposed to relying on a second pass to clean it up. (We may already have something like that that groups things to enforce order. I can't recall and didn't look. 🙃)

@nanjo712 nanjo712 force-pushed the feat/flush-in-lower-to-core branch from cec5065 to fc379a2 Compare May 28, 2026 06:52
@nanjo712
Copy link
Copy Markdown
Contributor Author

It seems better if this was handled as part of the LowerToHW pass pipeline as opposed to after

This is reasonable because a post-hoc pass appears to split the FIRRTL conversion process into two parts.

I tried to mimic the approach of addToAlwaysBlock to implement addToSimTriggeredBlock, and modified the way printf/fprintf generates sim.triggered accordingly. In this way, we should be able to allow FIRRTLToHW to directly generate a single sim.triggered block with guaranteed order, instead of what appears to be multiple concurrent sim.triggered blocks.

Look forward to your feedback!

@circt-bot
Copy link
Copy Markdown

circt-bot Bot commented May 28, 2026

Results of circt-tests run for fd64981 compared to results for 6360de5: no change to test results.

@fzi-hielscher
Copy link
Copy Markdown
Contributor

Hi @nanjo712,
I've brought up this topic in yesterday's open developer meeting and we've decided to reconsider my old PR (#7676, feel free to comment on that too) and finally figure out how we want to sequence operations in an HWModule. Apologies if that will delay progress a bit and we will likely have to backtrack on some of the things you have just implemented. But we'll hopefully then have a solid agreement on how we want to express sequences coming from FIRRTL.

@nanjo712
Copy link
Copy Markdown
Contributor Author

I've brought up this topic in yesterday's open developer meeting and we've decided to reconsider my old PR (#7676, feel free to comment on that too) and finally figure out how we want to sequence operations in an HWModule.

Thank you for discussing this topic on ODM; I have already took a look on #7676. I hope we can eventually reach a consensus regarding the modeling of the ordering of side-effect operations.

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.

4 participants