Skip to content

[CIR][LowerToLLVM][NFC] Refactor GlobalOpLowering for better readability and maintainability #1525

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

Merged
merged 2 commits into from
Mar 25, 2025

Conversation

seven-mile
Copy link
Collaborator

As noted in this comment, the nested if-arms in GlobalOpLowering are somewhat confusing and error-prone. This PR simplifies the logic into more straightforward components.

Since LLVM's GlobalOp accepts two types of initializers (either an initializer value or an initializer region), we've extracted the decision logic into a separate function called lowerInitializer. This function takes two inout arguments: mlir::Attribute &init (for the attribute value) and bool useInitializerRegion (as the decision indicator). All code paths then converge at a common epilogue that handles the operation rewriting.

The previous implementation for lowering DataMemberAttr initializers relied on recursion between MLIR rewrite calls, which made the control flow somewhat opaque. The new version makes this explicit by using a clear self-recursive pattern within lowerInitializer.

@bcardosolopes
Copy link
Member

The previous implementation for lowering DataMemberAttr initializers relied on recursion between MLIR rewrite calls

Perhaps at some point we should change this to some iteration algorithm instead.

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.

Awesome, thanks for the cleanup!

@bcardosolopes bcardosolopes merged commit cb15d3f into main Mar 25, 2025
9 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