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

[CIR] Remove return !cir.void from IR and textual representation #1249

Merged
merged 3 commits into from
Jan 10, 2025

Conversation

keryell
Copy link
Collaborator

@keryell keryell commented Dec 19, 2024

C/C++ functions returning void had an explicit !cir.void return type while not
having any returned value, which was breaking a lot of MLIR invariants when the
CIR dialect is used in a greater context, for example with the inliner.

Now, a C/C++ function returning void has no return type and no return values,
which does not break the MLIR invariant about the same number of return types
and returned values.

This change does not keeps the same parsing/pretty-printed syntax as before for
compatibility like in #1203 because it
requires some new features from the MLIR parser infrastructure itself, which is
not great.

This uses an optional type for function return type.

The default MLIR parser for optional parameters requires an optional anchor we
do not have in the syntax, so use a custom FuncType parser to handle the optional
return type.

@bcardosolopes
Copy link
Member

Hi @keryell, I missed this cause I usually don't go over draft PRs! Does this commit only reverts the original one or does it add anything else on top? I'm a bit confused by the description.

@bcardosolopes bcardosolopes marked this pull request as ready for review January 7, 2025 18:07
@keryell
Copy link
Collaborator Author

keryell commented Jan 8, 2025

I first reverted the original PR and I am still working on a good solution.
I got distracted by vacations 😄 and llvm/llvm-project#76337 since I am tweaking the CIR type .td file.

@keryell keryell force-pushed the ronan/remove-void-from-textual-IR branch from aa71766 to afde518 Compare January 8, 2025 17:25
@bcardosolopes
Copy link
Member

Can we have a PR that just reverts your previous solution? This might be easier to deal with next time we rebase. Another PR (or this one) can then reintroduce all the work without the MLIR bits?

@keryell
Copy link
Collaborator Author

keryell commented Jan 9, 2025

Can we have a PR that just reverts your previous solution? This might be easier to deal with next time we rebase. Another PR (or this one) can then reintroduce all the work without the MLIR bits?

Sure. I made #1276

@bcardosolopes
Copy link
Member

Landed

C/C++ functions returning void had an explicit !cir.void return type while not
having any returned value, which was breaking a lot of MLIR invariants when the
CIR dialect is used in a greater context, for example with the inliner.

Now, a C/C++ function returning void has no return type and no return values,
which does not break the MLIR invariant about the same number of return types
and returned values.

This change does not keeps the same parsing/pretty-printed syntax as before for
compatibility like in llvm#1203 because it
requires some new features from the MLIR parser infrastructure itself, which is
not great.

For now use a list of return types.
Move from a list of return types with 0 or 1 element to an optional type.
The default MLIR parser for optional parameters requires an optional anchor we
do not have in the syntax.
@keryell keryell force-pushed the ronan/remove-void-from-textual-IR branch from 0b4af61 to 94a19f6 Compare January 10, 2025 15:43
Copy link
Member

@bcardosolopes bcardosolopes left a comment

Choose a reason for hiding this comment

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

Waiting for tests to finish, but approach LGTM

@keryell keryell changed the title [CIR] Remove also the !cir.void return type from the textual IR [CIR] Remove return !cir.void from IR and textual representation Jan 10, 2025
@keryell
Copy link
Collaborator Author

keryell commented Jan 10, 2025

It was more subtle than expected because of MLIR TableGen OptionalParameter current limitations.

@bcardosolopes bcardosolopes merged commit 54d48d8 into llvm:main Jan 10, 2025
6 checks passed
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