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

[ClangIR][Lowering][NFC] Split LowerToLLVM.h from LowerToLLVM.cpp #1102

Merged
merged 1 commit into from
Nov 15, 2024

Conversation

ChuanqiXu9
Copy link
Member

This is a NFC patch that moves declaration from LowerToLLVM.cpp.

The motivation of the patch is, we hope we can use the abilities from MLIR's standard dialects without lowering ALL clangir operation to MLIR's standard dialects. For example, currently we have 86 operations in LowerToLLVM.cpp but only 45 operations under though MLIR. It won't be easy to add proper lowering for all operation to different dialects.

I think the solution may be to allow mixed IR. So that we can lowering CIR to MLIR's standard dialects partially and we can use some existing analysis and optimizations in MLIR and then we can lower all of them (the MLIR dialects and unlowered clangir) to LLVM IR. The hybrid IR is one of the goals of MLIR as far as I know.

NOTE: I completely understand that the DirectlyLLVM pipeline is the tier-1 pipeline that we want to support. The idea above won't change this. I just want to offer some oppotunities for the downstream projects and finally some chances to improve the overall ecosystem.

@bcardosolopes
Copy link
Member

I think this makes sense overall: regardless of how we setup pipelines, the functionality should be available in a more composable way, which this PR allows for. This is purely NFC, right?

I wonder if as part of this we should also rename the classes to be more descriptive with what they implement (because they might as well later be intermixed), for example, should we follow a pattern like CIRCopyOpLowering would become CIRToLLVMCopyOpLowering? @smeenai @lanza thoughts?

@bcardosolopes bcardosolopes changed the title [NFC] [clangir] [Lowering] Split LowerToLLVM.h from LowerToLLVM.cpp [ClangIR][Lowering][NFC] Split LowerToLLVM.h from LowerToLLVM.cpp Nov 11, 2024
@smeenai
Copy link
Collaborator

smeenai commented Nov 11, 2024

I'd be in favor of the more explicit names, especially if we'll potentially be mixing lowering paths in the future.

@ChuanqiXu9
Copy link
Member Author

Done renaming.

This is purely NFC, right?

Absolutely if I didn't make some oversight during the rebase. But even if there is any oversight, I believe it can be caught easily by the tests.

Copy link

github-actions bot commented Nov 12, 2024

✅ With the latest revision this PR passed the C/C++ code formatter.

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.

LGTM, will land once the format + conflicts are gone!

@ChuanqiXu9
Copy link
Member Author

LGTM, will land once the format + conflicts are gone!

Done!

@bcardosolopes bcardosolopes merged commit da6218b into llvm:main Nov 15, 2024
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.

3 participants