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

[ImportVerilog]: Create variables for function arguments #7829

Merged
merged 4 commits into from
Nov 21, 2024

Conversation

Max-astro
Copy link
Contributor

@Max-astro Max-astro commented Nov 16, 2024

Adds shadow variables for SV function arguments.
Fixes: #7431

Adjusts two test cases test/Conversion/ImportVerilog/basic.sv and test/Conversion/ImportVerilog/builtins.sv

Adds two test functions in test/Conversion/ImportVerilog/builtins.sv, which will crash in the former circt-verilog tool.

@Max-astro Max-astro changed the title [ImportVerilog]: Create variables for function arguments (#7431) [ImportVerilog]: Create variables for function arguments Nov 16, 2024
Copy link
Member

@hailongSun2000 hailongSun2000 left a comment

Choose a reason for hiding this comment

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

LGTM! Thank @Max-astro for your work on this error(#7431).

test/Conversion/ImportVerilog/basic.sv Outdated Show resolved Hide resolved
@hailongSun2000
Copy link
Member

@fabianschuiki: And if the user never assigns to the variable, the canonicalizers will get rid of it automatically again later 😃

If you want IR to stay the same when using --canonicalize like

func.func private @dummyE(%arg0: !moore.i1) -> !moore.i1 {
    return %arg0 : !moore.i1
}

You have to implement the VariableOp::Canonicalize() manually. But you can add this in the new PR 😄.

Copy link
Contributor

@fabianschuiki fabianschuiki left a comment

Choose a reason for hiding this comment

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

Awesome, thanks for implementing this!

If you want to get rid of all the changes in the SV tests, you could do what @hailongSun2000 has suggested and try to delete the variables again if they were not needed. That should make most tests see no real change at all, which may be beneficial? Let me know what you think 😃 I'm also very happy with just landing this!

lib/Conversion/ImportVerilog/Structure.cpp Show resolved Hide resolved
lib/Conversion/ImportVerilog/Structure.cpp Outdated Show resolved Hide resolved
Copy link
Contributor

@fabianschuiki fabianschuiki left a comment

Choose a reason for hiding this comment

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

Very cool, thanks a lot for doing all this work!

@Max-astro
Copy link
Contributor Author

Very cool, thanks a lot for doing all this work!

I really appreciate the guidance you provided, it means a lot to me that you took the time to help.

Copy link
Member

@hailongSun2000 hailongSun2000 left a comment

Choose a reason for hiding this comment

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

So cool!

@hailongSun2000
Copy link
Member

Can I merge it? @Max-astro

@Max-astro
Copy link
Contributor Author

Can I merge it? @Max-astro

Sure, I'm glad to hear that.

@hailongSun2000 hailongSun2000 merged commit 7fa8632 into llvm:main Nov 21, 2024
4 checks passed
@fabianschuiki
Copy link
Contributor

Fantastic, thanks for merging @hailongSun2000 and thanks for fixing this bug @Max-astro 🥳!

@Max-astro Max-astro deleted the import-verilog-fix branch November 22, 2024 01:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[ImportVerilog] Create variables for function arguments
3 participants