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][Lowering] Add scf.scope lowering to standard dialects #262

Merged
merged 1 commit into from
Sep 19, 2023

Conversation

Kuree
Copy link
Contributor

@Kuree Kuree commented Sep 16, 2023

This PR adds MLIR lowering of cir.scope.

I also notice that the MLIR unit tests still uses old integer types. I will fix those in a separate PR.

Copy link
Contributor

@htyu htyu left a comment

Choose a reason for hiding this comment

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

Please replace "[MLIR]" in the title with "[Lowering]". Thanks.


for (auto &block : scopeOp.getRegion()) {
rewriter.setInsertionPointToEnd(&block);
auto yield = cast<mlir::cir::YieldOp>(block.getTerminator());
Copy link
Contributor

Choose a reason for hiding this comment

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

Does every block in the scope have a yield? I recall that some blocks can have a return op. @bcardosolopes can you please confirm?

Copy link
Member

Choose a reason for hiding this comment

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

Right, possible terminators are YieldOp, ReturnOp, ThrowOp

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Got it. The doc seems to be a little outdated.

Copy link
Member

Choose a reason for hiding this comment

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

Created #265 to track this.

@Kuree Kuree changed the title [CIR][MLIR] Add scf.scope lowering to standard dialects [CIR][Lowering] Add scf.scope lowering to standard dialects Sep 19, 2023
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.

Requesting changes based on @htyu comments.

I'm ok with this direction, but if you're willing to go an extra mile, the ProperWay™ to do this is to unwrap cir.scope as part of LoweringPrepare.cpp pass, in a CIR to CIR transformation. This way both LLVM and MLIR lowering can share the same scope unwrapping logic.

If the region only has one block: then it's a matter of moving the alloca's to the parent region entry block. If there's more than one block than you need to also tie cleanup blocks.


for (auto &block : scopeOp.getRegion()) {
rewriter.setInsertionPointToEnd(&block);
auto yield = cast<mlir::cir::YieldOp>(block.getTerminator());
Copy link
Member

Choose a reason for hiding this comment

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

Right, possible terminators are YieldOp, ReturnOp, ThrowOp

@Kuree
Copy link
Contributor Author

Kuree commented Sep 19, 2023

I'm ok with this direction, but if you're willing to go an extra mile, the ProperWay™ to do this is to unwrap cir.scope as part of LoweringPrepare.cpp pass, in a CIR to CIR transformation. This way both LLVM and MLIR lowering can share the same scope unwrapping logic.

If the region only has one block: then it's a matter of moving the alloca's to the parent region entry block. If there's more than one block than you need to also tie cleanup blocks.

Are you referring to the cleanup/optimization steps before the CIR lowering? If I understand it correctly, don't you still need a lowering rewrite pattern for cir.scope, since you need to model the scoping semantics regardless?

@bcardosolopes
Copy link
Member

Are you referring to the cleanup/optimization steps before the CIR lowering?

I just realized that scf.scope is higher level and will benefit from direct lowering, nevermind!

@bcardosolopes bcardosolopes merged commit 3b400b1 into llvm:main Sep 19, 2023
@Kuree Kuree deleted the patch/scope-mlir branch September 25, 2023 20:59
lanza pushed a commit that referenced this pull request Oct 27, 2023
This PR adds MLIR lowering of `cir.scope`.

I also notice that the MLIR unit tests still uses old integer types. I
will fix those in a separate PR.
lanza pushed a commit that referenced this pull request Dec 20, 2023
This PR adds MLIR lowering of `cir.scope`.

I also notice that the MLIR unit tests still uses old integer types. I
will fix those in a separate PR.
lanza pushed a commit that referenced this pull request Jan 29, 2024
This PR adds MLIR lowering of `cir.scope`.

I also notice that the MLIR unit tests still uses old integer types. I
will fix those in a separate PR.
lanza pushed a commit that referenced this pull request Mar 23, 2024
This PR adds MLIR lowering of `cir.scope`.

I also notice that the MLIR unit tests still uses old integer types. I
will fix those in a separate PR.
eZWALT pushed a commit to eZWALT/clangir that referenced this pull request Mar 24, 2024
This PR adds MLIR lowering of `cir.scope`.

I also notice that the MLIR unit tests still uses old integer types. I
will fix those in a separate PR.
eZWALT pushed a commit to eZWALT/clangir that referenced this pull request Mar 24, 2024
This PR adds MLIR lowering of `cir.scope`.

I also notice that the MLIR unit tests still uses old integer types. I
will fix those in a separate PR.
lanza pushed a commit that referenced this pull request Apr 29, 2024
This PR adds MLIR lowering of `cir.scope`.

I also notice that the MLIR unit tests still uses old integer types. I
will fix those in a separate PR.
lanza pushed a commit that referenced this pull request Apr 29, 2024
This PR adds MLIR lowering of `cir.scope`.

I also notice that the MLIR unit tests still uses old integer types. I
will fix those in a separate PR.
eZWALT pushed a commit to eZWALT/clangir that referenced this pull request Apr 29, 2024
This PR adds MLIR lowering of `cir.scope`.

I also notice that the MLIR unit tests still uses old integer types. I
will fix those in a separate PR.
lanza pushed a commit that referenced this pull request Apr 29, 2024
This PR adds MLIR lowering of `cir.scope`.

I also notice that the MLIR unit tests still uses old integer types. I
will fix those in a separate PR.
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