[PATCH] D135427: [LTO] Make local linkage GlobalValue in non-prevailing COMDAT available_externally

[Teresa Johnson via Phabricator via llvm-commits]

tejohnson added inline comments.


================
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
----------------
MaskRay wrote:
> 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.
> 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.

I meant make the alias itself a declaration, not its aliasee. Since we are making it available_externally it must have a def elsewhere. Making it a declaration instead would avoid having to allow GlobalAlias to be available_externally in the verifier.

>> 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.

I think it would be the linker not the bitcode that would cause this situation to happen if at all? Not sure if we have a solid way to detect here what is prevailing (although if we have bumped the linkage from linkonce_odr to weak_odr then it was the prevailing copy). In the C5/D5 constructor alias case that failed, what was the linkage type before and after FinalizeInModule? The added test case has multiple weak_odr, but I would imagine that normally these would start being linkonce_odr?


================
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()) {
----------------
MaskRay wrote:
> 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...
Ok as long as it fails obviously somewhere. Wondering if we need a TODO here to handle that case in the future?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D135427/new/

https://reviews.llvm.org/D135427