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

Thu Oct 27 13:37:34 PDT 2022

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:
> > MaskRay wrote:
> > > tejohnson wrote:
> > > > 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.
> > > An available_externally alias preserve the most information: it is still an alias and the aliasee unknown. Changing GlobalAlias to a GlobalVariable/Function declaration changes the shape drops the information. To me an available_externally GlobalAlias is a better choice here.
> > > 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?
> > >
> > > 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.
> >  
> > Sorry, it's not DefinedGlobals.
> >  
> > `thinLTOInternalizeModule` is a pass after `thinLTOFinalizeInModule`.
> > `convertToDeclaration` does RAUW, but does not remove the `GlobalAlias` from the module `getAliasList`.
> > An unnamed `GlobalAlias` (due to `NewGV->takeName(&GV);` in `convertToDeclaration`) causes a failure in `thinLTOInternalizeModule=>internalizeModule=>Internalize.cpp:266=>Internalize.cpp:163 (`if (shouldPreserveGV(GV))`).
> > The function tries to acceoss the first byte of the name while the name is empty.
> >  
> > I think this can be fixed by replacing a GlobalAlias with a GlobalAlias/Function declaration. However, the GlobalAlias/Function approach does not scale when we ever need to support a GlobalAlias which references a ConstantExpr (instead of a GlobalValue).
> > An available_externally GlobalAlias can be extended to such a use case. Though I cannot think of a case Clang does such codegen.
> >  
> > > 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.
> >  
> > The new test file constructor-alias.ll describes the case. It is derived from this C++ source file https://reviews.llvm.org/D135427#3869003
> I see. Ok, I spent some time looking at the other callers of convertToDeclaration, since it seems as though they should have the same problem. The callsite in this file has a big FIXME (line 1107) that explains why we don't hit this - we currently explicitly avoid modifying GlobalAlias in thinLTOResolvePrevailingGUID, so that we don't end up with any that are even transiently available_externally, because the compiler doesn't handle that for aliases as we know also from your patch, so we never end up calling convertToDeclaration at that callsite for GlobalAlias.
> 
> The 2 other callsites are:
> - dropDeadSymbols: also called from the ThinLTO backend just before this method, follows up with the deletion of affected symbols from the module (for aliases since convertToDeclaration does a RAUW the old GV should never have any uses left, so it should always get erased from module there). 
> - filterModule: called when writing the bitcode at the very end of the pre-LTO link compile. It erases the original GV from the module if convertToDeclaration returns false, which it does in the case of an alias where the symbol was replaced. So aliases are handled correctly there.
> 
> Given the FIXME in the earlier callsite, I think it would be good to go ahead and just add support for deleting the alias when it has been converted to a decl. This avoids having to loosen up the constraints in the verifier. In theory there could be some lost opportunities e.g. for inlining, but I would guess that in the comdat case we would have been able to capitalize on most available inlining opportunities in the pre-LTO compile's inlining step and not need something like function importing first anyway, so dropping to a decl here and removing the old alias seems safest.
> 
> Ideally given the issue you hit it would be safest to delete the original alias in convertToDeclaration after the RAUW. Then dropDeadSymbols just needs to be changed somewhat since it is adding the symbol to a set before calling convertToDeclaration - it can just guard adding the symbol to the set based on the return value of convertToDeclaration. And the callsite in filterModule should be changed to skip the erasure and just ignore the return value. This will be a nice cleanup anyway.
> 
> > The new test file constructor-alias.ll describes the case. It is derived from this C++ source file https://reviews.llvm.org/D135427#3869003
> 
> Ok. I couldn't get it to generate the C5/D5 alias for me, even with cc1 -mconstructor-aliases, but in any case from the test case IR here it looks like they are getting generated as weak_odr. But also after looking at the FIXME from line 1107 I realized that because these are aliases we wouldn't have already modified their linkage type anyway.
> Ideally given the issue you hit it would be safest to delete the original alias in convertToDeclaration after the RAUW. Then dropDeadSymbols just needs to be changed somewhat since it is adding the symbol to a set before calling convertToDeclaration - it can just guard adding the symbol to the set based on the return value of convertToDeclaration. And the callsite in filterModule should be changed to skip the erasure and just ignore the return value. This will be a nice cleanup anyway.

A GlobalAlias can alias a GlobalAlias, ultimately (via the alias chain) it aliases to a GlobalVariable or Function or GlobalIFunc or ConstantExpr.  I see that `convertToDeclaration` can make GlobalAlias work for GlobalVariable/Function, and with some code GlobalIFunc can be supported, but how to support ConstantExpr?

available_externally GlobalAlias seems a natural extension and supports ConstantExpr well (if we find a need for it).

>> The new test file constructor-alias.ll describes the case. It is derived from this C++ source file https://reviews.llvm.org/D135427#3869003
>
> Ok. I couldn't get it to generate the C5/D5 alias for me, even with cc1 -mconstructor-aliases, but in any case from the test case IR here it looks like they are getting generated as weak_odr. But also after looking at the FIXME from line 1107 I realized that because these are aliases we wouldn't have already modified their linkage type anyway.

Sorry, updated https://reviews.llvm.org/D135427#3869003 to include a correct C++ example using `C5/D5`.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D135427