[PATCH] D129937: [JITLink][COFF] Handle duplicate external symbols.

[Lang Hames via Phabricator via llvm-commits]

lhames accepted this revision.
lhames added a comment.
This revision is now accepted and ready to land.

LGTM.



================
Comment at: llvm/lib/ExecutionEngine/JITLink/COFFLinkGraphBuilder.cpp:229
+            &G->addExternalSymbol(SymbolName, Sym->getValue(), Linkage::Strong);
+      GSym = ExternalSymbols[SymbolName];
     } else if (Sym->isWeakExternal()) {
----------------
sgraenitz wrote:
> Tracking all symbols names in new collection appears expensive. Could we check `LinkGraph::external_symbols()` instead?
`LinkGraph::external_symbols()` is a vector at the moment -- it won't scale well if there are lots of duplicates. I wonder if `LinkGraph` should have a `SmallDenseMap` structure -- vector if/when it has `<N` elements, otherwise a DenseMap?

That's a problem for another patch though. I think it's fine for `COFFLinkGraphBuilder` to use a separate map while we're bringing COFF support up, and we can tune it later. 


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

https://reviews.llvm.org/D129937