Skip to content

Conversation

smeenai
Copy link
Collaborator

@smeenai smeenai commented Oct 17, 2024

LLVM's verifier enforces this, which was previously causing us to fail
verification. This is a bit of a band-aid; the overall linkage and
visibility setting flow needs some work to match the original.

LLVM's verifier enforces this, which was previously causing us to fail
verification. This is a bit of a band-aid; the overall linkage and
visibility setting flow needs some work to match the original.
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, see comment as well

// TODO(cir): This needs a lot of work to better match CodeGen. That
// ultimately ends up in setGlobalVisibility, which already has the linkage of
// the LLVM GV (corresponding to our FuncOp) computed, so it doesn't have to
// recompute it here. This is a minimal fix for now.
Copy link
Member

Choose a reason for hiding this comment

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

Looks like this is subtle enough that it deserves an issue to track it?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yup, I filed #992 to fix this up more thoroughly.

@smeenai smeenai merged commit 03154f8 into llvm:main Oct 18, 2024
7 checks passed
@smeenai smeenai deleted the local-visibility branch October 18, 2024 02:58
keryell pushed a commit to keryell/clangir that referenced this pull request Oct 19, 2024
…lvm#990)

LLVM's verifier enforces this, which was previously causing us to fail
verification. This is a bit of a band-aid; the overall linkage and
visibility setting flow needs some work to match the original.
lanza pushed a commit that referenced this pull request Nov 5, 2024
…990)

LLVM's verifier enforces this, which was previously causing us to fail
verification. This is a bit of a band-aid; the overall linkage and
visibility setting flow needs some work to match the original.
lanza pushed a commit that referenced this pull request Mar 18, 2025
…990)

LLVM's verifier enforces this, which was previously causing us to fail
verification. This is a bit of a band-aid; the overall linkage and
visibility setting flow needs some work to match the original.
terapines-osc-cir pushed a commit to Terapines/clangir that referenced this pull request Sep 2, 2025
…lvm#990)

LLVM's verifier enforces this, which was previously causing us to fail
verification. This is a bit of a band-aid; the overall linkage and
visibility setting flow needs some work to match the original.
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