1. I will add a llvm-link tests for combining objc and swift bitcode.
2. What do you mean by "codegen tests for correct value in the object file"? There is no "object file" in the picture. Are you asking for a test that checks the value in the output of llvm-link?

Commited in: 9366397f057d18401e680b2cb28a0ee17c59d4a6

In D69893#1786222, @kamleshbhalui wrote:

In D69893#1786202, @steven_wu wrote:

The fix LGTM. Do you have a reproducer that can be used as a test case? We should really add more tests for libunwind.

I currently do not have a reproducer test case for this.

LLVM LTO part LGTM. I will get another pair of eyes on lld changes.

SGTM.

This sounds like a good idea. Have you thought about just write out upgraded bitcode file on disk (since you named it a "SymTabCache"), maybe into thinLTO cache? This will speed up the incremental build as well. I think most of the upgrade path are coming from linking a bitcode static library, which is probably not changing during incremental build so we just need to upgrade once.

The fix LGTM. Do you have a reproducer that can be used as a testcase? We should really add more tests for libunwind.

Address review feedback

Breaking part the meta data sounds like a good idea. You will probably need to combine the value in the backend to generate the field in the object file.

Thanks for increasing test coverage. One comment in line.

Ah, I miss your fix. The fix LGTM.

LGTM!

LGTM

In D70404#1755124, @evgeny777 wrote:

I reimplemented the patch, now using extra 'Constant' attribute in GlobalVarSummary.

The goal is to allow importing of constants, which are not being detected as 'ReadOnly'. Such constants are imported with available_externally linkage, still allowing compiler to make some optimizations along the way.

In D70397#1753789, @miyuki wrote:

In D70397#1753767, @steven_wu wrote:

In this patch, add another stack frame completely defeat the purpose of assert(unw_step(&cursor) > 0);.

Is the DWARF unwinder supposed to be always able to step out of main()?

In D70397#1753428, @broadwaylamb wrote:

This test won't compile for ARM Linux (using ARM EHABI), I get the error this directive must appear between .cfi_startproc and .cfi_endproc directives.

LGTM with one nitpick.

Fix comments

Address review feedback

ping

LGTM

I think this is probably the best fix I can think as well. Agree with Teresa says, please add comments for reasons why it is implemented this way.

LGTM.

I can see why this is a good idea, but do you have numbers for this optimization? How much code can be inlined? Benchmark for speedup? code size? compile time impact?

LGTM.

ccache does not have the support for this, I am just saying that this can be easily implemented in ccache and that would be much better than the proposed solution here.

In D69327#1719411, @ychen wrote:

Thanks for the inputs @steven_wu @tejohnson. Totally agree with the points you brought up. One last thing I'm not quite sure is the caching of -fthin-link-bitcode. It is a -cc1 option since it is a kind of implementation of ThinLTO, right? I'm a little hesitant to begin writing up patches to teach build system/caching tool (I could think of at least three for our workload) to recognize this option because of that. If there are any changes to the option, the same thing needs to be done again. Do you have any thoughts on that? Is the option in use for your workload and do you think it is stable enough to have build systems caching for it? (Another option is to produce -fthin-link-bitcode output post compile time which I assume having total build time impact to some degree).

LGTM

Can you clarify what do you mean by 'waste time optimizing a file that finally hit the object file cache'?

That is not exactly what I meant. I meant:

LGTM. Thanks!

In D69107#1714175, @evgeny777 wrote:

From https://llvm.org/docs/LangRef.html#comdats :

Comdats have a name which represents the COMDAT key. All global objects that specify this key will only end up in the final object file if the linker chooses that key over some other key

To enforce this LLVM internalize pass doesn't internalize entire COMDAT group if any of its members should be preserved (see shouldPreserveGV). At the moment we can internalize global
in llvm::thinLTOResolvePrevailingInModule without looking at other COMDAT group members and break this guarantee

A small suggestion for improvement.

Sounds like a good plan going forward.

I think this change is generally good but sorry I am not quite familiar with the semantics of comdat. Can you explain in more detail why that can be a problem to internalize that in promote?

Thanks for working on this. Comments inline.

This sounds fine to me. We should probably replace most of report_fatal_error with llvm::Error and propagate them correctly. In the meantime, having better crash is an improvement and does not conflict with what we want to do eventually.

Some high level comments before I look into details. Are we expecting those two APIs to co-exist? Maybe just create a new libLTO and replacing the existing one in few releases? Maybe libLTO.1.dylib and libLTO.2.dylib, with a cmake configuration for which one to symlink to?

I break this into two commits. r372021 and r372022.

• [LTO][Legacy] Add new C inferface to query libcall functions
• [lld] Update lld to use the new LTO API to add libcall symbols

In D67568#1670049, @kledzik wrote:

How many symbols are there today? Do we want this overhead of a function call for each? Could you instead return a count and array pointer (like argc and argv) or have a "foreach" function that takes a block and calls it back for each symbol?

lto.cpp is not really tested. Maybe we can test this by move this into lib/LTO then integrate the new version into lld?

• [LTO][Legacy] Add new C inferface to query libcall functions

In D67565#1669913, @tejohnson wrote:

In D67565#1669823, @steven_wu wrote:

Sorry, should be clear that this is for doxygen documents. See @defgroup @ingroup from the comments. Those functions where in the Caching APIs group.

I don't see LLVM website is generating documents based on groups but still good to get it correct.

Just want to clean this up because I am planning to add some new APIs.

I get why they don't belong in the caching section, but now they are in the ThinLTO-specific section (LLVMCTLTO). Should they be in the earlier LLVMCLTO section which seems to include more of the generic LTO interfaces?

Sorry, should be clear that this is for doxygen documents. See @defgroup @ingroup from the comments. Those functions where in the Caching APIs group.

The diagnostics message looks good to me.

Hi Teresa, I am seeing some regressions because of this commit. ThinLTO is now producing more weak symbols than before and I am looking for a good fix. See inline comments for details.

We can discuss what is a good class name for TapiFile and TapiUniversal. It should be consistent with the other patch as well.

few small comments. Otherwise, LGTM.

LGTM