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

[RTG] Add set union operation #7916

Merged
merged 1 commit into from
Dec 9, 2024
Merged

[RTG] Add set union operation #7916

merged 1 commit into from
Dec 9, 2024

Conversation

maerhart
Copy link
Member

No description provided.

@maerhart maerhart added the RTG Involving the `rtg` dialect label Nov 28, 2024
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.

LGTM

Comment on lines 150 to 151
Computes the union of the given sets. If the list of sets must contain at
least one element.
Copy link
Contributor

Choose a reason for hiding this comment

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

Does the second sentence want to say "The list of sets must contain at least one element."? If yes, it might be worth checking for that in the verifier, or if empty sets are valid, maybe just return an empty set in that case?

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 that's what it should say. It originally said that if there are zero operands, it returns the empty set but then changed it because the SameOperandsAndResultType trait verifies that there is at least one operand (there is also a test for that already added). We could go back to allowing zero operands, but I'm not sure it it's beneficial (it would just fold to a zero-operand set_create).

Copy link
Contributor

@fabianschuiki fabianschuiki Dec 4, 2024

Choose a reason for hiding this comment

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

Oh nice! I was wondering where the error message came from 😀 No need to change anything, disallowing empty operand lists feels very reasonable. Especially since you'd have two different ways of creating a constant empty set otherwise, which would be pretty weird.

@maerhart maerhart force-pushed the maerhart-rtg-register-interface branch from 6990196 to f5204eb Compare December 5, 2024 09:42
@maerhart maerhart changed the base branch from maerhart-rtg-register-interface to main December 5, 2024 09:43
@maerhart maerhart force-pushed the maerhart-rtg-set-union branch from 2bf1d4d to d4c18d8 Compare December 6, 2024 15:28
@maerhart maerhart changed the base branch from main to maerhart-rtg-elaboration-pass December 6, 2024 15:28
@maerhart maerhart force-pushed the maerhart-rtg-set-union branch from d4c18d8 to f61ea9d Compare December 6, 2024 15:32
Base automatically changed from maerhart-rtg-elaboration-pass to main December 9, 2024 12:45
@maerhart maerhart force-pushed the maerhart-rtg-set-union branch from f61ea9d to 9e4d714 Compare December 9, 2024 13:55
@maerhart maerhart merged commit 2aecdf7 into main Dec 9, 2024
4 checks passed
@maerhart maerhart deleted the maerhart-rtg-set-union branch December 9, 2024 14:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
RTG Involving the `rtg` dialect
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants