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

Tue Oct 25 14:11:15 PDT 2022

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
----------------
rnk wrote:
> MaskRay wrote:
> > 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.
> > 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.
> 
> It doesn't sound so bad to me, but maybe I don't have the right visibility. As it stands, I'd rather have complex code than a less accurate IR model.
> 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.

Sorry, you lost me. What does DefinedGlobals have to do with calling convertToDeclaration, which takes a GV? The GlobalAlias below is a GV, why can't we just call convertToDeclaration on it instead of making it available_externally?

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

convertToDeclaration handles converting alias to either a variable or function, and the underlying methods used to get one of those does add it to the appropriate list on the module. Not sure about removing from the alias list, but it does do the RAUW.

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

I agree they should all get the same prevailingness, if things are working as expected, but it sounds like they are in different comdat groups. I'm trying to understand what the linkage type was because if it started out as linkonce_odr the thin link should convert to either available_externally if not prevailing or weak_odr if prevailing.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D135427