Skip to content

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
terapines-osc-cir pushed a commit to Terapines/clangir that referenced this pull request Sep 2, 2025
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.
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