Can you add a bit more explanation other than just saying fixing PR38372?

In D49434#1170267, @pcc wrote:

I would imagine that folks who care about code size would be linking with --gc-sections (or -dead_strip in ld64) so the library functions would end up being dropped if they aren't actually needed.

For the old API, I don't think it is reasonable to expect user or linker to pass in the correct preserve list. There is likely no knowledge outside the LTO modules that these lib funcs are needed.

In D49434#1165761, @tejohnson wrote:

In D49434#1165612, @pcc wrote:

You wouldn't be making changes to the IR, only the summary. I was thinking that you might add code to http://llvm-cs.pcc.me.uk/lib/Analysis/ModuleSummaryAnalysis.cpp#534 that would effectively do:

if (ValueInfo VI = Index.getValueInfo(GlobalValue::getGUID("memcpy")))
    for (auto &Summary : VI.getSummaryList())
      Summary->setLive(true);
if (ValueInfo VI = Index.getValueInfo(GlobalValue::getGUID("memmove")))
    for (auto &Summary : VI.getSummaryList())
      Summary->setLive(true);
// etc.

If I recall correctly it wasn't the LTO dead code elimination that was removing them. Caroline, I forget what was happening in the original case - were they being internalized by LTO and then eliminated by GlobalDCE? If so, then Peter's approach should fix the issue by preventing them from being internalized (at least by ThinLTO, not sure about regular LTO).

In D49362#1163775, @tejohnson wrote:

In D49362#1163768, @steven_wu wrote:

When importing a variable definition when the ref has this bit set, it can be imported as a local copy, without promoting the name/linkage type. That will avoid the need to do any modification to the optimization passes. On the exporting side, we could do prevent the promotion if we knew that all modules importing a reference also imported the referenced variable definition. It does however look like we should be importing all non-interposable linkage constant variables that are referenced, which should be all we care about for this anyway.

What if the constant is really big? Looks like the importing decision has to be made by the thin linker directly. You can not import the "constant" global as something like "available_externally constant" because the original copy might not be visible from the original module (just like the example), unless you want to modify the original copy to be "external hidden".

Not sure I understand your question, can you clarify the concern? Right now, computeImportForReferencedGlobals during the thin link we import any referenced variable, unless it has interposable linkage or references other summaries (in which case it is not a constant). The imported variables are imported as available externally definitions. The original definition is currently promoted to external hidden.

When importing a variable definition when the ref has this bit set, it can be imported as a local copy, without promoting the name/linkage type. That will avoid the need to do any modification to the optimization passes. On the exporting side, we could do prevent the promotion if we knew that all modules importing a reference also imported the referenced variable definition. It does however look like we should be importing all non-interposable linkage constant variables that are referenced, which should be all we care about for this anyway.

LGTM. Thanks!

Add testcase using 6.0 bitcode.

Address review feedback

oops, accidentally opened a new review. closing.

In that case, we can enable verification on checked-in bitcode files as a follow-up. How about testing this by round-tripping textual IR through llvm-as and llvm-dis? You just need a function, alias, and global var with local linkage and without dso_local set, right?

In D49039#1154801, @vsk wrote:

Thanks for doing this!

Could you add a test? Adding "opt -verify <old-bitcode-file>" run lines to the compatibility tests would be fine.

It is easier and cleaner if I just fold everything into getOSVersion.

handle *-apple-macosx target option

In D48849#1150246, @arphaman wrote:

Hmm, the driver should not call inferDeploymentTargetFromArch when -target is passed. Or am I missing something?

Rebase the commit correctly

Update patch. Use a better API.

Unfortunately, I wasn't able to write a test for this because the host triple in the configuration can either be x86_64-apple-darwin* or x86_64-apple-macosx*, but the one used passed by driver is always macosx one. I can't reliably compare those two.

In D48783#1148427, @tejohnson wrote:

In D48783#1148412, @steven_wu wrote:

Does the iteration order of those map matters other than changing the hash? Will it change symbol resolution or importing different function?

No effect on importing/symbol resolution etc. In fact, this is not an input to the compiler at all, it is used by the build system. The change in order will just make it look to the build system like it has changed when it hasn't.

Does the iteration order of those map matters other than changing the hash? Will it change symbol resolution or importing different function?

LGTM.

I am not working on lld but this looks good for me. I like this solution better than the alternative you mentioned.

I think it is easier to unregister all the signal handlers in the beginning of the llvm_shutdown() since you should be done with all the work when you call llvm_shutdown(). This can also allow safe access ManagedStatics in signal handler, which I don't know if we have more of those other than the lock you are trying to fix. The only difference might be that if an interrupt is triggered when freeing ManagedStatics, llvm signal handler will not be used.

Are you planning to upstream the module pass to justify this to be part of Support library?

Thanks for working on this Jonas. Duncan and Adrian has already covered pretty much all I want to say. A small additional comments.

Address review feedback

Address review feedback

Address review feedback. Fix typos and comments.

Thanks for reviewing Erik!

LGTM. Thanks!

LGTM with some feedback inline.

I am not trying to discuss which english word is best here. My point is simply:

1. macros are evaluated during compile time
2. "host"means either the platform you compiled on during compile time or the platform you run on during the runtime
3. __is_host_* is not a good name, because it is misleading as it either implies "runtime" as a compile-time constant, or indicates the wrong platform.

It is not about matching command line name to builtin marco name. "target" is the platform we are compiling for, whether it is host or device or something else. It is a different concept when you talking about cross-compiling, which "target" is strictly not host and "build" or "host" doesn't matter to compiler at all.

I disagree. I think "target" is the correct name, even for cross compiling. For something compiled with -target foo, we are consistent calling it "target" during compile time. It only becomes appropriate to call it "host" during the runtime of the executable. There is no such concept as "host" when you are doing cross compiling.

If we all agree that dropping local_unnamed_addr is invalid, I can abandon this. Thanks for the feedback!

I might have missed something. Can you be more specific about how it is going to work?

The solution is to use canBeOmittedFromSymbolTable from include/llvm/Object/IRSymtab.h.

The symbol table is computed earlier and so the above predicate still returns 1. Your examples work with lld and ThinLTO if you want to check the implementation details.

So the original of this patch is thinLTO is promoting linkonce_odr to weak_odr. Think the following two situations:

Additional comment, I didn't realize llvm-nm now prioritize to print bitcode symbol table, rather than the symbol table from the object file. There should be an option to choose which one to see and maybe also an option to see both.

Thanks for fixing this. LGTM.

In D43959#1024157, @kubamracek wrote:

Thanks, yes, AFAIK, ld64 is fine and I don't really know how to test lld on Darwin (is it even supposed to work?).

The idea is to prevent LTO (ConstMerge pass) from merging global constants that has the same value, which has the same side-effect as linker merging two weak symbols that has ODR violation.

LGTM. And yes, order doesn't matter.

The API change is fine for ld64. But you need to do the following when you update LTO C API:

• You need to declare the interface in include/llvm-c/lto.h
• You need to bump LTO_API_VERSION in that header and document the new API is available since the new LTO_API_VERSION.
• You need to update tools/lto/lto.exports. This is the export list used by ld64 and only the interface listed in that file will be exported on darwin platform.

Michael doesn't have commit right yet so I will commit this for him.

I am using local_unnamed_addr as an example. Maybe you are right but I think my point is still valid. That is, LTO optimization and code generation can modify the symbol table from the bitcode file. It is already the case that symbols can be internalized, removed, backend can also insert new symbols. Unless there is a rule saying LTO passes cannot modify the linkage or visibility or any other attributes to the symbols, linker cannot trust whatever the bitcode symbol table says and firmly it will not be changed for some good reason.

I think this approach is much better. Can you update the title because it is not really a version bump anymore? From my understanding, this is good enough for backwards-compatibility. Thanks!

In D43361#1009616, @pcc wrote:

There is an alternative which is to teach linker to deduct auto hide property from the bitcode file and overwrite the linkage type afterwards. For C API, there is a new scope LTO_SYMBOL_SCOPE_DEFAULT_CAN_BE_HIDDEN can be used to do that but that will be forced the work onto all the linker which is probably not desirable but it solves the same problem.

This seems better to me. Ideally the linker should be reading and resolving symbols from bitcode files in the same way as regular object files, including the auto-hide bit. Linkers would need something similar for any other properties that are contained only in bitcode files but not in object files (such as a general address-taken bit).

In D43361#1009486, @pcc wrote:

The last time this was attempted (D28978) we established that the linker can do this itself. I don't think the situation has changed since then.

Generated by mistake. Abandon and I will reformat the patch.

Move the check later so it is only check when needed.

In D42267#999761, @kromanova wrote:

I am definitely not against the change. It would be good to have the number means the same thing in C/C++ API. I think ld64 can be updated in advance before we release 7.0 so we can have a larger compatibility window so I don't think this should be a blocker.

So, can I assume that I got an "OK" from the other main stakeholder who is using C LTO API? BTW, I CC-ed you to another small patch https://reviews.llvm.org/D42446, where I added a couple of missing APIs for controlling cache policy. You might want to expose it to ld64 users.

In D42267#999711, @kromanova wrote:

In D42267#999581, @steven_wu wrote:

This indeed affects how ld64 interact with libLTO. If user didn't specify a value, ld64 will always call thinlto_codegen_set_cache_pruning_interval with interval 0. This is currently a no-op so it will use whatever the default value in libLTO. With this change, it will be force pruning.

It is possible for us to make change to future ld64 to use different value but it is not going work very well for compatibility. If old ld64 loaded a new libLTO, it is going to lose incremental build.

Well, the main reason for this change was:

(a) compatibility with C++ API. A lot of code in libLTO is common for C API and C++ API. Having the same semantics for the same options in two different APIs will make code simpler and prevents from someone who only cares about C+ API to unintentionally break the C API functionality. The latter is particularly important, since incremental linking is not thoroughly tested.

(b) giving a possibility to a user to run a pruner immediately, vs. "no-op" which could get achieved simply by not passing "pruning-interval" option to the linker, making option value 0 useless.

I understand your compatibility issue, but I don't think it's very serious.

• I doubt it's common for a user to pass a option to the linker, if the same effect could be achieved by not passing any option at all (why would user do extra work?).
• This option will cause the pruner to run, which not necessarily means that the cache files will get removed and incremental build won't happen. Cache files will get removed only if at the same time the files are stale, or their sizes are larger then certain thresholds, which are passed as parameters.
• The pruner runs just 20 minutes earlier than it would have run by default. So, only the builds that are done more frequent than 20 minutes might be affected.
• Only the users who pick up the linker and libLTO from different packages will get affected.
  
  Let me know what you decide. If you want to keep things the old way, I could modify this patch for PS4 only.
  
  So, to summarize, we could add a meaningful value for pruning_interval option, which currently can't get achieved otherwise, and additionally we will offer compatibility with C++API. The only users that will be affected are the ones who because of whatever reason decided to explicitly specify a default option on the command line rather than skipping it, whose builds are happening more frequently than 20 minutes and who had mixed and matched different releases of ld64 and libLTO.

This indeed affects how ld64 interact with libLTO. If user didn't specify a value, ld64 will always call thinlto_codegen_set_cache_pruning_interval with interval 0. This is currently a no-op so it will use whatever the default value in libLTO. With this change, it will be force pruning.