[PATCH] D104986: [ThinLTO] Respect ClearDSOLocalOnDeclarations for unimported functions

[Fangrui Song via Phabricator via llvm-commits]

MaskRay added inline comments.


================
Comment at: llvm/lib/Transforms/Utils/FunctionImportUtils.cpp:130
+    // to an imported externally visible global value.
+    if (!SGV->isDeclarationForLinker() && !isa<GlobalAlias>(SGV))
       return GlobalValue::AvailableExternallyLinkage;
----------------
tejohnson wrote:
> I don't follow what is happening with and without this change. If we aren't planning to import something as a definition, why do we need to mark it available_externally in the source module? How does this change result in dropping the dso local flag?
> 
> From your summary:
> > i.e. when isPerformingImport, an unimported define dso_local @foo should be treated as an available_externally function, instead of an ExternalLinkage definition. 
> 
> It should eventually end up as an external declaration. Is that not happening? Or is the available_externally marking just a temporary adjustment to trigger subsequent dso_local clearing?
I got confused. The few cases where getLinkage returns `ExternalLinkage` means this is an imported declaration (I thought it means an unimported definition).

Returning `AvailableExternallyLinkage` is not good - we will get `declare available_externally` and available_externally shouldn't be used on declarations. This is benign because bitcode writer treats available_externally declaration like an external declaration.

I've reverted the part.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D104986