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

[FIRRTL][Sim] Add fopen and fclose intrinsics, add fd operand to printf #7111

Closed
wants to merge 3 commits into from

Conversation

SpriteOvO
Copy link
Member

This PR enables the SystemVerilog generated by Chisel&CIRCT can be fwrite to different files by the simulator.

FIRRTL Spec changes: chipsalliance/firrtl-spec#213 (some discussion already there)

Closes #7092, CC @seldridge @sequencer @uenoku

@SpriteOvO SpriteOvO force-pushed the fir-file-io-intrinsic branch 2 times, most recently from bf6d9dd to cf39a6b Compare June 1, 2024 01:28
@SpriteOvO SpriteOvO marked this pull request as draft June 1, 2024 01:34
@SpriteOvO SpriteOvO force-pushed the fir-file-io-intrinsic branch from cf39a6b to 04767b0 Compare June 1, 2024 01:40
Comment on lines +4681 to +4682
return setLoweringTo<sv::SystemFunctionOp>(
op, resultTy, builder.getStringAttr("fopen"), ValueRange{filename, mode});
Copy link
Member

Choose a reason for hiding this comment

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

$fopen is task so you cannot put this into non-procedural region. Please put this to initial block and assign return value to a register.

Comment on lines +4687 to +4690
auto result = builder.create<sv::SystemFunctionOp>(
NoneType::get(builder.getContext()), builder.getStringAttr("fclose"),
ValueRange{fd});
return success();
Copy link
Member

Choose a reason for hiding this comment

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

This also needs to be in a procedural region. I noticed currently there is no support for final statement. That seems reasonable addition to SV dialect.

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.

Generally the implementation looks great but let's wait for how chipsalliance/firrtl-spec#213 goes first 👍

StringRef formatString;
if (parseExp(clock, "expected clock expression in printf") ||
parseExp(condition, "expected condition in printf") ||
parseExp(fd, "expected fd expression in printf") ||
Copy link
Member

@uenoku uenoku Jun 1, 2024

Choose a reason for hiding this comment

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

It 's necessary to accept old syntal. Please add a condition for FIRRTL-spec version.

Comment on lines +10 to +12
node fd = intrinsic(circt_fopen<filename = "file.txt", mode = "a"> : SInt<32>)
printf(clock, cond, fd, "test %d\n", var)
intrinsic(circt_fclose, fd)
Copy link
Member

Choose a reason for hiding this comment

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

This is pretty problematic as this is introducing a procedural ordering requirement that is not reflected in the IR. I can legally move the close before the printf, yet there is an expectation that I do not and that this will lower to a specific order in Verilog.

Copy link
Member

Choose a reason for hiding this comment

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

Ah, I was assuming fclose intrinsic is lowered in fclose in final statement. Alternatively we could add clock and enable operands either.

@SpriteOvO
Copy link
Member Author

Closed as in favor of #7983.

@SpriteOvO SpriteOvO closed this Jan 7, 2025
@SpriteOvO SpriteOvO deleted the fir-file-io-intrinsic branch January 7, 2025 08:29
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.

[FIRRTL][Sim] Add fopen and fclose
3 participants