[PATCH] D135427: [LTO] Make local linkage GlobalValue in non-prevailing COMDAT available_externally
Fangrui Song via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Mon Oct 24 22:41:24 PDT 2022
MaskRay marked an inline comment as done.
MaskRay added inline comments.
================
Comment at: llvm/lib/IR/Verifier.cpp:849
+ GA.hasAvailableExternallyLinkage(),
+ "Alias should have available_externally, private, internal, linkonce, "
+ "weak, linkonce_odr, "
----------------
tejohnson wrote:
> Would probably need to update https://llvm.org/docs/LangRef.html#aliases too.
> Should we be checking that if alias is available_externally, then its aliasee is as well?
`LLParser.cpp still rejects available_externally aliases.` so `available_externally alias` is currently for LTO internal use, not exposed in `.ll` files.
================
Comment at: llvm/lib/Transforms/IPO/FunctionImport.cpp:1160
+ Changed = false;
+ // If an alias references a GlobalValue in a non-prevailing comdat, change
+ // it to available_externally. For simplicity we don't handle ConstantExpr
----------------
tejohnson wrote:
> Would an alternative be converting the alias to a declaration? Slight pessimization in optimization ability but the verifier and other documentation doesn't need to be change.
>
> But what if this was the prevailing copy of the alias? Could that happen?
Currently a GlobalAlias cannot alias a declaration or an available_externally. We need to make an extension and I feel that allowing available_externally is a better choice as it enables optimizations.
> But what if this was the prevailing copy of the alias? Could that happen?
If a bitcode does something unsupported thing like making one comdat prevailing while another comdat non-prevailing and creating references between the two, such an unsupported case can happen. There is probably not much we can do and I don't think we need extra efforts to detect the case.
================
Comment at: llvm/lib/Transforms/IPO/FunctionImport.cpp:1162
+ // it to available_externally. For simplicity we don't handle ConstantExpr
+ // aliasee, which is unlikely used in a COMDAT.
+ for (auto &GA : TheModule.aliases()) {
----------------
tejohnson wrote:
> Should that be asserted?
The case technically can happen and will cause a Verifier failure, so we don't necessarily assert it here...
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D135427/new/
https://reviews.llvm.org/D135427
More information about the llvm-commits
mailing list