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

[Fangrui Song via Phabricator via llvm-commits]

MaskRay marked an inline comment as done.
MaskRay 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
----------------
tejohnson wrote:
> MaskRay wrote:
> > tejohnson wrote:
> > > 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?
> > > 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.
> > 
> > An alias cannot be a declaration.
> > 
> > If we use a function or variable, we need to figure out the aliasee of the alias to decide whether to use GlobalVariable or a Function. This is more work than using an available_externally alias.
> > 
> > If a `C5/D5` constructor is non-prevailing, we expect that everything in the non-prevailing `$_ZN1AD2Ev` and the associated `$_ZN1AD5Ev` to be non-prevailing. The current code satisfies this property.
> > An alias cannot be a declaration.
> > 
> > If we use a function or variable, we need to figure out the aliasee of the alias to decide whether to use GlobalVariable or a Function. This is more work than using an available_externally alias.
> 
> Right, I meant drop to a variable or function declaration. We already have a helper to do so, defined in this file (see convertToDeclaration), that handles the alias case. Actually, the comments at the callsite (in this function)  points out a case where we may need to do this for correctness (original linkage type is interposable) - not sure if that can ever happen here but would be good to handle correctly or assert.
> 
> > If a C5/D5 constructor is non-prevailing
> 
> But we don't know for sure here whether it is prevailing or not, I think? In the C5/D5 case that was failing, what were the linkage types before and after FinalizeInModule above?
Unfortunately, `convertToDeclaration` is insufficient (this fact motivated me to use available_externally alias). It is for GlobalValues which will not call  `auto GS = DefinedGlobals.find(GV.getGUID());` in `thinLTOInternalizeModule` (as an empty name triggers a fault in getGUID). Non-prevailing GlobalAlias can exercise the fault code path.

If we really want to use a declaration, we need to make distinction of GlobalVariable/Function and remove the old GlobalAlias in `TheModule.getAliasList()` and add a new one to `TheModule.getGlobalList()` (or the Function counterpart). This is lots of complexity.

>> If a C5/D5 constructor is non-prevailing
>
> But we don't know for sure here whether it is prevailing or not, I think? In the C5/D5 case that was failing, what were the linkage types before and after FinalizeInModule above?

We know for sure. `C2/C5` must either be all prevailing or all non-prevailing. If we the available_externally code path is triggered for one alias in `C2`, then we know everything in `C2/C5` should be available_externally as well. I verified this with some internal targets which motivated me to revert the previous patch.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D135427