Skip to content

[CIR] Simlipify string literal global creation #1632

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 3 commits into from
May 22, 2025

Conversation

andykaylor
Copy link
Collaborator

Previously, when emitting a global for a string literal, we were creating a GlobalOp, building a GlobalView attr for it, and looking up the global from the symbol associated with the attr. This change splits out the function that creates the global so that the global is returned directly and the GlobalView attribute is only created in the case where it is needed.

This also updates the mechanism used for uniquing the global name used for the strings so that if different base names are used the uniquing numbers each base name separately. The mangling of the global used for strings is not implemented, but the uniquing was happening prior to the mangling. This change drops the uniquing below the placeholder for mangling.

Previously, when emitting a global for a string literal, we were creating
a GlobalOp, building a GlobalView attr for it, and looking up the global
from the symbol associated with the attr. This change splits out the
function that creates the global so that the global is returned directly
and the GlobalView attribute is only created in the case where it is needed.

This also updates the mechanism used for uniquing the global name used for
the strings so that if different base names are used the uniquing numbers
each base name separately. The mangling of the global used for strings is
not implemented, but the uniquing was happening prior to the mangling.
This change drops the uniquing below the placeholder for mangling.
@andykaylor
Copy link
Collaborator Author

These changes reflect similar changes made in llvm/llvm-project#140796

Copy link
Collaborator

@xlauko xlauko left a comment

Choose a reason for hiding this comment

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

I would expect a test to see uniquing works as expected for different base names.

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, pending testcase - or is this NFCI for the current feature set? If so maybe just add that to the PR title instead.

@andykaylor
Copy link
Collaborator Author

LGTM, pending testcase - or is this NFCI for the current feature set? If so maybe just add that to the PR title instead.

I hesitate to call it NFC, because different things happen under the hood, but there is no change in the results, so in that sense it is NFC.

@bcardosolopes bcardosolopes merged commit 3793010 into llvm:main May 22, 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.

3 participants