In D151965#4391631, @steven_wu wrote:

The only way I can think of to test that is to use llvm-lto with --exported-symbol=, which preserves that symbol via ThinLTOCodeGenerator::preserveSymbol.

So the new API users don't use the exported symbol function? Then current test case looks fine to me.

In D151965#4391234, @steven_wu wrote:

I think this should work for legacy API. ld64 always picks the prevailing one to be from native object file and ask LTO to reserve the symbol (so LTO cannot internalize them) so it doesn't really hit the bug mentioned. It is not the ideal solution but it is the best can be done when no symbol resolution information is available from API.

It should be safe to assume atom only gets referenced once if there is only one visible and it is not in the reserved symbol list. I don't think preserved symbols will turn into prevailing, but I don't think they can be internalized in this case too. Would be good to add a test case to make sure if the weak/linkonce in the preserve GUID will get the correct behavior.

lgtm

lgtm

Thanks @mingmingl for digging into the effects!

Just a few minor comments/suggestions

Can we do this during the importing process? E.g. somewhere in FunctionImportGlobalProcessing::processGlobalForThinLTO.

Address comment

In D149932#4334052, @tejohnson wrote:

In D149932#4334051, @shchenz wrote:

empty hot-alloctype.patch is still there, the file looks like a typo?

Yes, it looks like that was accidentally added into the patch and got committed. I will remove that.

In D149932#4334051, @shchenz wrote:

empty hot-alloctype.patch is still there, the file looks like a typo?

In D149932#4327394, @tejohnson wrote:

In D149932#4327378, @dyung wrote:

@kanwu, this change seems to break on a Windows build bot, can you take a look and revert if you need time to investigate? (I'm not sure why you were not listed in the changes for this build on buildbot)
https://lab.llvm.org/buildbot/#/builders/216/builds/20923

I committed it for Kan, odd that the buildbots don't seem to be including this change specifically. Looks like some platforms don't recognize "uint". I will commit a fix in a few mins to use "unsigned" instead.

In D149932#4327378, @dyung wrote:

@kanwu, this change seems to break on a Windows build bot, can you take a look and revert if you need time to investigate? (I'm not sure why you were not listed in the changes for this build on buildbot)
https://lab.llvm.org/buildbot/#/builders/216/builds/20923

lgtm

Rebase

Rebase on top of the committed D149924.

Looks great, thanks! Just a few minor comments.

Reopening so I can eventually upload fix for instability surfaced by expensive checks bot.

Address comments

Previously @yamauchi did a bunch of work related to this called PGSO (Profile Guided Size Optimization). See the functions in https://github.com/llvm-mirror/llvm/blob/master/lib/Transforms/Utils/SizeOpts.cpp, which are used to guide a number of optimizations that can affect code size. Do you know why those weren't sufficient?

Fix for nondeterminism.

Reverted in 6fbf022908c104a380fd1854fb96eafc64509366 due to bot failure: https://lab.llvm.org/buildbot/#/builders/245/builds/7888

Address comments

In D148950#4293068, @ormris wrote:

No, I don't think we need to modify the original test. It's now used to test the changes in d2ef8c1f.

Address comments

Also noticed the comments in the new tests for MemProfContextDisambiguation refer to the old version of the lifetime threshold option, updating those too.