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][NFC] Add test rejecting connect of properties. #5491

Merged
merged 1 commit into from
Jun 27, 2023

Conversation

dtzSiFive
Copy link
Contributor

No description provided.

Copy link
Member

@seldridge seldridge left a comment

Choose a reason for hiding this comment

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

Great. Thanks for adding tests to lock in this behavior.

Comment on lines +1270 to +1271
// expected-error @below {{must be a passive base type}}
firrtl.strictconnect %out, %0 : !firrtl.string
Copy link
Member

Choose a reason for hiding this comment

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

Error message isn't awesome here. However, this is locking in existing errors. So... 👍

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh, good call, was splitting from other work so didn't think too hard 😇 . Thanks for pointing this out, regardless! You're right this is just locking in existing behavior, but on this.....

I wonder how we could generate better error messages for violations of ODS constraints?
We could drop them in some places (darn) in favor of manual verifiers where we control the diagnostics,
but they're nice to have in ODS and are somewhat necessary/significant directly (MLIR detects if you're using a constraint derived from X to know to generate/reason about Y in a certain way, so on (e.g., SameTypeOperands, TypesMatchWith).

We could in some places decompose them into compositions of more specific errors (not a property type, so on)... 🤔

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Looks like we already produce nice diagnostic for this if encountered during parsing, so unlikely user will see this, phew! 👍 .

@dtzSiFive dtzSiFive merged commit c09770b into llvm:main Jun 27, 2023
@dtzSiFive dtzSiFive deleted the feature/test-prop-connect branch June 27, 2023 15:50
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.

2 participants