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

[Pirama Arumuga Nainar via Phabricator via llvm-commits]

pirama marked 4 inline comments as done.
pirama added inline comments.


================
Comment at: lib/LTO/LTO.cpp:353
     } else if (EnableLTOInternalization &&
                !GlobalValue::isLocalLinkage(S->linkage()))
       S->setLinkage(GlobalValue::InternalLinkage);
----------------
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.


Repository:
  rL LLVM

https://reviews.llvm.org/D54125