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] Check unknown width and reset rules during parsing 4.0.0. #6731

Merged
merged 2 commits into from Feb 23, 2024

Conversation

dtzSiFive
Copy link
Contributor

@dtzSiFive dtzSiFive commented Feb 21, 2024

Only check during parsing to stay compatibile with previous behavior.

Allow abstract reset on extmodule until spec changes.

if (ftype.hasUninferredWidth())
return emitError(module.getPortLocation(idx),
"public or ext module port must have known width");
// Presently abstract reset on extmodule is not explicitly disallowed in
Copy link
Contributor

Choose a reason for hiding this comment

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

That seems like a deficiency in the spec, not the intended behavior. I would be ok with updating the spec and enforcing this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Adding the checks to the parse per current spec behavior, will extend once that is made illegal there.

Only check during parsing to stay compatibile with previous behavior.

Allow abstract reset on extmodule until spec changes.
@dtzSiFive dtzSiFive changed the title [FIRRTL] Verify public and extmodule requirements about uninferred width and reset [FIRRTL] Check unknown width and reset rules during parsing 4.0.0. Feb 21, 2024
@dtzSiFive dtzSiFive marked this pull request as ready for review February 21, 2024 23:33
Copy link
Contributor

@mikeurbach mikeurbach left a comment

Choose a reason for hiding this comment

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

Makes sense to me.

lib/Dialect/FIRRTL/Import/FIRParser.cpp Outdated Show resolved Hide resolved
@dtzSiFive dtzSiFive merged commit 2d5b6d1 into llvm:main Feb 23, 2024
4 checks passed
@dtzSiFive dtzSiFive deleted the fix/public-known-types branch February 23, 2024 14:30
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.

None yet

3 participants