[PATCH] D51199: CodeGen: Add two more conditions for adding symbols to the address-significance table.

[Peter Collingbourne via Phabricator via llvm-commits]

pcc added inline comments.


================
Comment at: llvm/lib/CodeGen/AsmPrinter/AsmPrinter.cpp:1546
+      if (!GV.use_empty() && !GV.isThreadLocal() &&
+          !GV.hasDLLImportStorageClass() && !GV.getName().startswith("llvm.") &&
           !GV.hasAtLeastLocalUnnamedAddr())
----------------
smeenai wrote:
> rnk wrote:
> > rnk wrote:
> > > Dllimport things aren't actually always imported, sometimes they can be resolved to symbols in the same image. MSVC will warn for this by default, but in my experience developers disable the warning, especially if they don't use Windows for development.
> > > 
> > > Can we end up with an incomplete table if we apply dllimport, resolve the symbol locally, and take the symbols address in that TU?
> > To answer my own question, maybe we just don't worry about doing extra ICF on symbols like that. Basically, if you have code like this, you see this warning, and then disable it, and *then* you rely on the symbol's address being unique, you are out of luck.
> > 
> > These symbols tend to be the result of template instantiation anyway, which often have non-unique addresses with -fvisibility-inlines-hidden is set, which is pretty common.
> Even if a dllimported symbol ends up being found in the same image, you'll still end up going through the `__imp_` thunk, so the argument for the IAT entry never being address-significant should still apply?
Either that or we could pessimise the case where code uses a locally-imported symbol. That is, we should set `KeepUnique` on a symbol's section when we create a `DefinedLocalImport` for it.


Repository:
  rL LLVM

https://reviews.llvm.org/D51199