ping.

In D63735#1588025, @smeenai wrote:

In D63735#1587999, @steven_wu wrote:

This adds an unnecessary static initializer to the MachOObject.cpp. Can you move the array of validArchs into filescope? You can declare it in getValidArchs() and use getValidArchs() in isValidArchs.

I was hoping that std::array wouldn't necessitate a static initializer, since it's just supposed to be a zero-cost wrapper around a C array.

This adds an unnecessary static initializer to the MachOObject.cpp. Can you move the array of validArchs into filescope? You can declare it in getValidArchs() and use getValidArchs() in isValidArchs.

In D29044#1576319, @thakis wrote:

You need to add a llvm/test/tools/llvm-objdump/lit.local.cfg that contains config.suffixes = ['.yaml'], else the .yaml files added in this change won't be run by lit.

This added an extra dependency from Analysis -> TransformUtils. I don't think this dependency is a good idea, so is adding preheader in analysis pass.

LGTM.

Sounds like an interesting optimization. Is the global storage (from the other module) also eliminated in this case? If so, can you add a testcase for that?

SGTM

We should look at either merging the implementations or providing an api function that provides access to the "llvm.linker.options" named metadata via the IRSymtab. It may not be possible to merge the implementations because in COFF and MACHO the representation of dependent libraries is a string of command line options; whereas, the ELF representation is an array of library strings. This is because for COFF and MACHO there is basically only one linker for each file format so the command line format is fixed. For ELF there are a greater variety of linkers, so I just pass the library strings through to the linker, without processing them into command line arguments, and it is deferred to the linker to interpret the library strings.

This is ELF specific and miss the initial RFC so there is quite some reading to catch up. Should we merge the MachO implementation together with the ELF one? Once you commit this, you might want to file a bug report on me to catch up on the macho side.

LGTM.

Rebase the patch to TOT. Ping again.

Thanks for doing this. I think module flag is a good idea. Some comments inline.

In D61627#1497919, @wanders wrote:

Why are you interested in expending this list?

I have a (kernel) that is compiled with -mno-red-zone and -mcmodel=large which I want to compile with -fembed-bitcode for a debugging tool that needs the bitcode.

But I can carry these patches locally for now.

Like I said, I am not worried that -mno-red-zone itself changes the ABI. As long as LLVM still respect the attribute the same way, it is fine. I want to consult Tim to make sure we can support re-targeting no red zone from armv7k to arm64_32.

In D61627#1493674, @compnerd wrote:

@steven_wu - yeah, -mred-zone, -mno-red-zone does impact the ABI, at least on x86_64. It changes the way that the arguments are spilled and the stack layout.

The main concern for adding this blacklist was because of long term maintainability since the option is going to be embedded into bitcode. Looking at this specific option, I have no reason against because it doesn't affect embedded compiler flags.

When you are manually configuring the bootstrap, I still don't think trying to bypass libtool is the correct fix because you still need to specify the correct llvm-ar to use. The default CMAKE_AR inferred by cmake will not work. Maybe it is an option to add the control to force llvm-ar if wished, then you have two options when you manually configuring bootstrap:

Rebase the patch and did some fixup:

If you use cmake variable CLANG_ENABLE_BOOTSTRAP, it should take care of DYLD_LIBRARY_PATH so libtool is using the just built libLTO to create static library. The setup of the DYLD_LIBRARY_PATH is just below.

In D61255#1484855, @evgeny777 wrote:

I agree this doesn't look like the correct fix, especially the check !Sym.isExecutable() which doesn't look correct to me

The problem is that linkonce_odr functions and data should be IMO treated differently. The linkonce_odr linkage implies linker leaves only one copy of the symbol in the final image.
However it's not really a problem having multiple internal copies of linkonce_odr function because all copies follow one definition rule. This fact allows ThinLTO to efficiently optimize such function calls.
Situation however is different with linkonce_odr objects, because it's wrong to have multiple copies of linkonce_odr object because reads and writes supposed to access same memory will actually become
inconsistent. However there is still a nuance: if object is a compile time constant there can't be any write access to it, so we can safely internalize it.

I don't see this happening in legacy thinLTO code generator but I need to do more digging to see what is the difference. For legacy API, the static variable is not internalized or turned into available_externally.

ping

Add tests for the change. There is acutally an easy to test with llvm-lto

In D60924#1473166, @dexonsmith wrote:

In D60924#1473161, @steven_wu wrote:

In D60924#1473135, @dexonsmith wrote:

Is there a good way to test this?

Not currently in LLVM. It might be possible to add a test in clang repo which requires DARWIN and ld64.

Maybe we can split out the function to generate the filename, and add a unit test for that?

In D60924#1473135, @dexonsmith wrote:

Is there a good way to test this?

forgot to save the inline comments.

LGTM with one additional small comments

Thanks Teresa and Ben for the feedback! I am landing this change.

In D60421#1464989, @bd1976llvm wrote:

Sorry for the delay in replying. I was trying to understand if the upgrade path was important. Comments inline:

In D60421#1461442, @steven_wu wrote:

In D60421#1461407, @bd1976llvm wrote:

Hi Steven,

I missed that this work was being done - sorry.

Currently the legacy api does not require the IRSymtab to be written to the bitcode files so there is potentially a performance degradation due to this approach and the generated bitcode files will be larger, did you measure that?

The legacy API is not writing out IRSymtab but build that in memory during code generation. The only requirement change is that now data layout is required for thin LTO .

My understanding is that the on disk bitcode contains a binary representation of the IRSymtab and this is read into memory when loading the file (which is why the upgrade path is more expensive than the no-upgrade path). I am saying that currently the legacy api does not require the IRSymtab to be written to disk. Writing it may cost more time and make the input files bigger.

In D60421#1461407, @bd1976llvm wrote:

Hi Steven,

I missed that this work was being done - sorry.

Currently the legacy api does not require the IRSymtab to be written to the bitcode files so there is potentially a performance degradation due to this approach and the generated bitcode files will be larger, did you measure that?

Add one file that was missing from previous patch

In D60421#1458858, @tejohnson wrote:

What part of the patch caused the need to change the internalize action to do promote+internalize in one go?

I reverted this because the old thinLTO test cases need to be updated to have data layout as a requirement to use IRSymtab. I will start a separate review to address all those issues.

I reverted this because the old thinLTO test cases need to be updated to have data layout as a requirement to use IRSymtab. I will start a separate review to address all those issues.

Review feedback.

I talked with Akira offline and we think this is probably the best approach to fix this LTO issue. I will leave others to comment if they think otherwise.

Address review feedback

LGTM once the clang patch is good to land.

In D60226#1455688, @steven_wu wrote:

In D60226#1455485, @pcc wrote:

In D60226#1455477, @tejohnson wrote:

In D60226#1453856, @steven_wu wrote:

Also do note, I did a quick benchmark on thin-link time for libLTO.dylib. thin-link was 2.6s before the change vs. 4.0s after the change. Building IRSymtab during thin-link does introduce non-negligible overhead. It might be possible to build them async in threads to reduce the overhead.

@pcc is this surprising? Typically we should just be loading it out of the bitcode file. There are cases where it has to be rebuilt, but theoretically those should be exceptions.

Yes, we normally just load it out of the bitcode file. The irsymtab only needs to be rebuilt when the version of the producer and the consumer do not match. You might be hitting the "upgrade" path here: http://llvm-cs.pcc.me.uk/lib/Object/IRSymtab.cpp#330

This is likely. I didn't bootstrap the build. I will try again with a matching version.

In D60226#1455485, @pcc wrote:

In D60226#1455477, @tejohnson wrote:

In D60226#1453856, @steven_wu wrote:

Also do note, I did a quick benchmark on thin-link time for libLTO.dylib. thin-link was 2.6s before the change vs. 4.0s after the change. Building IRSymtab during thin-link does introduce non-negligible overhead. It might be possible to build them async in threads to reduce the overhead.

@pcc is this surprising? Typically we should just be loading it out of the bitcode file. There are cases where it has to be rebuilt, but theoretically those should be exceptions.

Yes, we normally just load it out of the bitcode file. The irsymtab only needs to be rebuilt when the version of the producer and the consumer do not match. You might be hitting the "upgrade" path here: http://llvm-cs.pcc.me.uk/lib/Object/IRSymtab.cpp#330

Also do note, I did a quick benchmark on thin-link time for libLTO.dylib. thin-link was 2.6s before the change vs. 4.0s after the change. Building IRSymtab during thin-link does introduce non-negligible overhead. It might be possible to build them async in threads to reduce the overhead.

In D59945#1449696, @theraven wrote:

I am not convinced that there is sufficient abstraction here to handle multiple runtimes without significant code duplication. The Mach-O and LLVM IR parsers both appear to duplicate all knowledge of the runtime's data structures, so if you wanted to add a second runtime you'd then need four classes. I would expect to see two abstraction layers:

1. Something allowing you to specify a data structure to examine that would load it from some binary (IR, Mach-O, ELF, whatever).
2. Something built on top of this that then queries specific runtime metadata structures.
   
   In clang, we support two broad families of Objective-C runtime: Apple (Legacy and Modern, with version-specific things for macOS, iOS, and so on) and GNU (including GCC, GNUstep / WinObjC, and ObjFW). These are largely orthogonal to object file formats. For example, the GNUstep runtime is widely used with both ELF and PE/COFF binaries and can be used with Mach-O (though typically only for testing). If we wanted to add support for all of these, I suspect that we'd have a code explosion.

Thanks for explanation. Then the current approach in this patch LGTM. Thanks!

In D59709#1440113, @tejohnson wrote:

In D59709#1440081, @steven_wu wrote:

In D59709#1440033, @tejohnson wrote:

In D59709#1440017, @steven_wu wrote:

I am not quite sure what is the semantics for ELF linker. When thinLTO picks the linkonce_odr version and promote it to weak, it is not changing the visibility. It is a later linker optimization that auto hide the linkonce_odr symbol, correct?

It is in fact ThinLTO that is changing the visibility (D43130). See the change I made to FunctionImport.cpp.

Interesting, I made that change! But I somehow remembered I got pushed back because of ELF semantics and end up implementing that in ld64 instead. Let me dig up some context and get back to you.

Are you thinking of https://reviews.llvm.org/D43361?

In D59709#1440033, @tejohnson wrote:

In D59709#1440017, @steven_wu wrote:

I am not quite sure what is the semantics for ELF linker. When thinLTO picks the linkonce_odr version and promote it to weak, it is not changing the visibility. It is a later linker optimization that auto hide the linkonce_odr symbol, correct?

It is in fact ThinLTO that is changing the visibility (D43130). See the change I made to FunctionImport.cpp.

I am not quite sure what is the semantics for ELF linker. When thinLTO picks the linkonce_odr version and promote it to weak, it is not changing the visibility. It is a later linker optimization that auto hide the linkonce_odr symbol, correct?

In D44373#1436308, @jyknight wrote:

In D44373#1036189, @steven_wu wrote:

Thanks for fixing this. LGTM.

Looks like this never actually got committed?

LGTM

LGTM.

I talked to Erik offline. I think the upgrade tests should run on .bc file rather than text format.

LGTM with a suggestion to make code cleaner.

Sorry for following up late on the patch. Removing the reachability testing for the exit block causes false positive for infinite loop cases like this:

Not sure about gn part and if that has any impact. Otherwise, LGTM.

Fix the comment

I was planning to add a test but I am not sure how to check the file type of temporary files.

In D53945#1347562, @thakis wrote:

In D53945#1343312, @thakis wrote:

Out of interest, why aren't most of the tests for this lit tests? The usual llvm way of testing stuff is to write a small llvm tool using your library (which will be useful outside of a testing context too) and then use that to write lit tests.

Looks like this is now happening (rL350341). Are most of the unit tests going to move to lit tests now?

Thanks for the clarifications. Would it be possible to utilize ThinBackendProc for this instead of a new CodeGenManager class? I.e. make a new derived version that does the index file write and spawns the local processes? The advantage is that it would start converging the implementations. And I think this could aid in refactoring suggested below to avoid duplication. Another advantage is that both LTO API's would have access to all backend implementations (in process, write indexes and exit, write indexes and use local processes, etc).

Other than a small design choice commented inline, LGTM.

See comments inline.