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][NFC] Move LoweringPrepare into CIRGen #1092

Merged

Conversation

lanza
Copy link
Member

@lanza lanza commented Nov 8, 2024

Move LP into CIRGen and give it a handle on the CIRGenModule. A lot of
code has been duplicated from CIRGen into cir/Dialect/Transforms in
order to let LP live there, but with more necessary CIRGen features
(e.g. EH scope and cleanups) going to be used in LP it doesn't make
sense to keep it separate. Add this patch that just refactors
LoweringPrepare into the CIRGen directory and give it a handle on the
CGM.

Created using spr 1.3.5
@lanza lanza requested a review from bcardosolopes as a code owner November 8, 2024 21:31
Copy link

github-actions bot commented Nov 8, 2024

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

Created using spr 1.3.5
@bcardosolopes
Copy link
Member

When you proposed this we discussed factoring out the CGM bits necessary and just passing it to LoweringPrepare, I'm curious what your findings were or at least some indication of what are the bits you really need there that justify this change, so far I have found none in the description of this PR.

CIRGen features (e.g. EH scope and cleanups)

This doesn't sound right, I've implemented most bits in CFGFlatten without needing CGM

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.

Marking as blocked on the question in previous comment

@bcardosolopes
Copy link
Member

I actually dig up a bit myself and this makes a lot of sense, I see now the point about code duplication - let's start with this and later we can factor out pieces as we go. Thanks for the refactoring.

@bcardosolopes bcardosolopes merged commit 4463352 into main Nov 9, 2024
6 checks passed
bcardosolopes added a commit that referenced this pull request Nov 9, 2024
@bcardosolopes
Copy link
Member

Unfortunately I had to revert: I was taking one more deep review in tree and it's just super weird that none of the previous passes need CGM, including things that can done on top of CIR without AST support, and some later pass depend on CGM. My take is that the needed pieces in CGM need to be factored out and reused between LP and CGM, this is probably the most clean way to go about this, we can chat more about it early next week. Sorry for the churn.

lanza added a commit to lanza/llvm-project that referenced this pull request Dec 11, 2024
Move LP into CIRGen and give it a handle on the CIRGenModule. A lot of
code has been duplicated from CIRGen into cir/Dialect/Transforms in
order to let LP live there, but with more necessary CIRGen features
(e.g. EH scope and cleanups) going to be used in LP it doesn't make
sense to keep it separate. Add this patch that just refactors
LoweringPrepare into the CIRGen directory and give it a handle on the
CGM.

Pull Request: llvm/clangir#1092
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