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

[MooreToCore] Fix parse error for parameter #7253

Merged
merged 4 commits into from
Jul 2, 2024

Conversation

mingzheTerapines
Copy link
Contributor

img_v3_02cc_cfdc615a-56e2-43ae-ab4e-f56bcd8c03dg
Fix parser error like this. Parser will find type after the ":", while I naming parameter with "@typekind:name". So replace ":" with "_" will solve this problem.

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.

Why are we adding inner symbols to constants in the first place 🤔 ?

@mingzheTerapines
Copy link
Contributor Author

@uenoku , SystemVerilog provides three elaboration-time constants: parameter, localparam, and specparam. Conversation should not lose these key words, so I added it at the firstplace, which won't duplicate with each other.

@fabianschuiki
Copy link
Contributor

If I recall correctly, the idea is to have the MooreToCore lowering preserve the constants as hw.wire such that they show up in simulations. It's pretty hacky. Ideally we'd do this with a dbg.variable, but there isn't very good support for those in Arcilator and other CIRCT lowerings yet.

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.

Fix LGTM!

@hailongSun2000 hailongSun2000 merged commit 281dc6c into llvm:main Jul 2, 2024
4 checks passed
@mingzheTerapines mingzheTerapines deleted the mingzhe-fixParam branch July 2, 2024 01:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants