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

[ExportVerilog] Support sv.func.* op emission #7015

Merged
merged 7 commits into from
Jun 9, 2024
Merged

Conversation

uenoku
Copy link
Member

@uenoku uenoku commented May 9, 2024

This PR (separated from #6977) implements ExportVerilog support of sv.func, sv.func.dpi.import, sv.func.call and sv.func.call.procedural.

sv.func emission is similar to hw.module but one difference is a return value. Surprisingly in SV a function name is used as a placeholder for a return value so name legalization properly sets hw.verilogName of a returned value to a function name.

Result names of sv.func.call.procedural are attached to temporary registers created in PrepareForEmission. This is similar implementation approach as hw.instance.

A part of PrepareForEmission and ExportVerilog is ported from #4856

Copy link
Member

@dobios dobios left a comment

Choose a reason for hiding this comment

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

Not really an expert on this, but apart from a few nits, the fact that you should update this wrt. the llvm bump fixes and llvm itself to make sure that it won't break on merge, and clang-tidy, LGTM.

if (dyn_cast_or_null<HWInstanceLike>(op.getSrc().getDefiningOp()))
// prepare assigns wires to instance outputs and function results, but these
// are logically handled in the port binding list when outputing an instance.
if (isa_and_nonnull<HWInstanceLike, FuncCallOp>(op.getSrc().getDefiningOp()))
Copy link
Member

Choose a reason for hiding this comment

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

nit: Is the second type argument really necessary?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, it's necessary. FuncCall op emissions follows a similar emission implementation to HWInstanceOp. The assignments are emitted in FuncCallOp emission so we need to skip here.

Comment on lines +4190 to +4328
auto printArg = [&](Value value) {
if (needsComma)
ps << "," << PP::space;
emitExpression(value, ops);
needsComma = true;
};
Copy link
Member

Choose a reason for hiding this comment

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

won't this lead to a trailing comma? Could you maybe do something using the argument index instead to figure out if you need a comma or not?

Copy link
Member Author

Choose a reason for hiding this comment

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

Good question! I think it will not produce training comma since a comma is always emitted before an operand.

ps << *linkageName << PP::nbsp << "=" << PP::nbsp;
auto op =
cast<FuncOp>(state.symbolCache.getDefinition(importOp.getCalleeAttr()));
assert(op.isDeclaration() && "function must be a declaration");
Copy link
Member

Choose a reason for hiding this comment

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

I don't think I've seen this way of adding assert messages before lol

Copy link
Member Author

Choose a reason for hiding this comment

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

startStatement();
ps.addCallback({func, true});
// A function is moduled as an automatic function.
emitFunctionSignature(*this, ps, func, /*isAutomatic=*/true);
Copy link
Member

Choose a reason for hiding this comment

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

nit: commented code

Copy link
Member Author

Choose a reason for hiding this comment

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

Adding an argument name as a comment is encouraged for boolean parameter: https://llvm.org/docs/CodingStandards.html#comment-formatting

@uenoku uenoku requested a review from darthscsi as a code owner May 16, 2024 09:31
@uenoku uenoku changed the base branch from dev/hidetou/dpi-import to dev/hidetou/dpi-import-old May 16, 2024 09:33
@uenoku uenoku force-pushed the dev/hidetou/func-export branch 2 times, most recently from ef06699 to 3767a5b Compare June 4, 2024 14:32
uenoku and others added 2 commits June 5, 2024 02:07
Co-authored-by: Will Dietz <will.dietz@sifive.com>
Base automatically changed from dev/hidetou/prepare-nfc to main June 6, 2024 15:33
@uenoku uenoku changed the title [ExportVerilog] Support sv.func.* emission [ExportVerilog] Support sv.func.* op emission Jun 9, 2024
@uenoku
Copy link
Member Author

uenoku commented Jun 9, 2024

I'd like to merge. Post-merge review is also appreciated!

@uenoku uenoku merged commit dc70062 into main Jun 9, 2024
4 checks passed
@uenoku uenoku deleted the dev/hidetou/func-export branch June 9, 2024 14:45
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