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

Pirama Arumuga Nainar via Phabricator via llvm-commits llvm-commits at lists.llvm.org
Wed Nov 7 11:27:00 PST 2018


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:
> > 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.
> I see - at HEAD the marking of the InternalLinkage in the summary has no effect on these appending variables since thinLTOInternalizeModule invokes llvm::internalizeModule which in turn special cases these @llvm.* special variables.
> 
> My preference then would be to make a change in the internalization handling, rather than work around this with a chance in the weak resolution (now prevailing) handling. So let's try to understand what is causing the failures you noted with the above patch applied. I looked at the first 3 test cases:
> 
> LLVM :: ThinLTO/X86/module_asm2.ll
> 
> This one has @llvm*.used appending variables. What is the failure mode when you apply your patch?
> 
> LLVM :: tools/gold/X86/comdat.ll
> LLVM :: tools/gold/X86/visibility.ll
> 
> The above two don't have any appending variables, so I'm not sure why they are affected. These 2 tests fail without a recent version of gold with a bugfix - are they passing for you without the above patch applied?
module_asm2.ll fails the check on %t.0.1.  Symbol `b`, the one added to `llvm.used` no longer gets exported.

```
../llvm/test/ThinLTO/X86/module_asm2.ll:37:12: error: NM0-DAG: expected string not found in input
; NM0-DAG: d b
           ^
<stdin>:1:2: note: scanning from here
 U b
 ^
```

The tools/gold/* failures are due to the unpatched ld.gold.  I knew that but forgot to remove it when I made the comment - sorry. 


Repository:
  rL LLVM

https://reviews.llvm.org/D54125





More information about the llvm-commits mailing list