[PATCH] D54125: [LTO] Drop non-prevailing definitions for non-local linkage types

[Pirama Arumuga Nainar via Phabricator via llvm-commits]

pirama added inline comments.


================
Comment at: lib/LTO/LTO.cpp:353
     } else if (EnableLTOInternalization &&
                !GlobalValue::isLocalLinkage(S->linkage()))
       S->setLinkage(GlobalValue::InternalLinkage);
----------------
tejohnson wrote:
> pirama wrote:
> > pirama wrote:
> > > If I skip appending-linkage summaries here, so I can instead get rid of the AppendingLinkage check in FunctionImport.cpp:thinLTOResolveWeakForLinkerModule (line 914), I get some test failures.
> > > 
> > > My reasoning was that this line sets linkage to Internal for summaries of appending-linkage symbols, which then gets propagated to the actual symbol in FunctionImport.cpp:thinLTOResolveWeakForLinkerModule.  Not setting the linkage here will obviate the need for that check.
> > > 
> > > I just realized this line is slightly different from my prior test - I must not have synced recently.  I'll recreate this scenario and report on the tests that failed.
> > On top of the current patch, if I apply the diff below:
> > ```
> > diff --git a/lib/LTO/LTO.cpp b/lib/LTO/LTO.cpp
> > index 2c3c7359dc0..347f1a56a25 100644
> > --- a/lib/LTO/LTO.cpp
> > +++ b/lib/LTO/LTO.cpp
> > @@ -350,7 +350,8 @@ static void thinLTOInternalizeAndPromoteGUID(
> >        if (GlobalValue::isLocalLinkage(S->linkage()))
> >          S->setLinkage(GlobalValue::ExternalLinkage);
> >      } else if (EnableLTOInternalization &&
> > -               !GlobalValue::isLocalLinkage(S->linkage()))
> > +               !GlobalValue::isLocalLinkage(S->linkage()) &&
> > +               !GlobalValue::isAppendingLinkage(S->linkage()))
> >        S->setLinkage(GlobalValue::InternalLinkage);
> >    }
> >  }
> > diff --git a/lib/Transforms/IPO/FunctionImport.cpp b/lib/Transforms/IPO/FunctionImport.cpp
> > index 5816633db97..f7c380c8a09 100644
> > --- a/lib/Transforms/IPO/FunctionImport.cpp
> > +++ b/lib/Transforms/IPO/FunctionImport.cpp
> > @@ -912,7 +912,6 @@ void llvm::thinLTOResolveWeakForLinkerModule(
> >      }
> >  
> >      if (GlobalValue::isLocalLinkage(GV.getLinkage()) ||
> > -        GlobalValue::isAppendingLinkage(GV.getLinkage()) ||
> >          // In case it was dead and already converted to declaration.
> >          GV.isDeclaration())
> >        return;
> > ```
> > 
> > the following tests fail:
> > 
> > ```
> >     LLVM :: ThinLTO/X86/module_asm2.ll                                                    
> >     LLVM :: tools/gold/X86/comdat.ll                                                      
> >     LLVM :: tools/gold/X86/visibility.ll                                                  
> >     cfi-devirt-lld-thinlto-x86_64 :: cross-dso/stats.cpp                                  
> >     cfi-devirt-thinlto-i386 :: cross-dso/stats.cpp                                        
> >     cfi-devirt-thinlto-newpm-i386 :: cross-dso/stats.cpp                                  
> >     cfi-devirt-thinlto-newpm-x86_64 :: cross-dso/stats.cpp                                
> >     cfi-devirt-thinlto-x86_64 :: cross-dso/stats.cpp                                      
> >     cfi-standalone-lld-thinlto-x86_64 :: cross-dso/stats.cpp                              
> >     cfi-standalone-lld-thinlto-x86_64 :: stats.cpp                                        
> >     cfi-standalone-thinlto-i386 :: cross-dso/stats.cpp                                    
> >     cfi-standalone-thinlto-i386 :: stats.cpp                                              
> >     cfi-standalone-thinlto-newpm-i386 :: cross-dso/stats.cpp                              
> >     cfi-standalone-thinlto-newpm-i386 :: stats.cpp                                        
> >     cfi-standalone-thinlto-newpm-x86_64 :: cross-dso/stats.cpp                            
> >     cfi-standalone-thinlto-newpm-x86_64 :: stats.cpp                                      
> >     cfi-standalone-thinlto-x86_64 :: cross-dso/stats.cpp                                  
> >     cfi-standalone-thinlto-x86_64 :: stats.cpp  
> > ```
> > 
> > If the diff above is preferable over the current patch, I'll look further into why the tests break.
> I'd like to understand what is causing the appending linkage variable to be internalized if you don't guard against appending linkage in the thinLTOResolveWeakForLinkerModule. At HEAD, we skip appending linkage in the thinLTOResolveWeakForLinker.* routines since it isn't weak linkage, and don't need to guard against them in the thinLTOInternalizeAndPromote* either.
> 
> With your change to computePrevailingCopies, any appending linkage should be prevailing, since they won't be in the map, and thinLTOResolveWeakForLinkerGUID should theoretically not do anything since they aren't LinkOnceLinkage - which should be the same net result as the code at HEAD for those variables. Why is it now being eventually marked internal in the index whereas before it apparently wasn't?
The reason is that in thinLTOInternalizeAndPromoteGUID, in the lines around the current comment, the Summary is marked as having InternalLinkage.  Subsequently, in thinLTOResolveWeakForLinkerGUID , the Summary's linkage is read from the DefinedGlobals parameter.  In that function, NewLinkage is InternalLinkage and gets set for the symbol itself.


================
Comment at: lib/Transforms/IPO/FunctionImport.cpp:898
       return;
     auto NewLinkage = GS->second->linkage();
     if (NewLinkage == GV.getLinkage())
----------------
The InternalLinkage set in thinLTOInternalizeAndPromoteGUID is read here.


================
Comment at: lib/Transforms/IPO/FunctionImport.cpp:942
                         << "\n");
       GV.setLinkage(NewLinkage);
     }
----------------
If I don't have the guard for AppendingLinkage earlier in this function, control flow reaches this line and the variable's linkage gets set to InternalLinkage.


Repository:
  rL LLVM

https://reviews.llvm.org/D54125