changed flag name to -lto-obj-path:

Landed in r368612 / d47622.

'll call the Config field ltoObjPath, same as in the ELF linker.

After discussion in person, we decided on -lto-obj-path: similar to the naming in the ELF linker, and we'

@rnk, what do you think of the name I proposed?

In ELF, the config name is ltoObjPath, and the flag is -plugin-opt=obj-path=

Unfortunately, this causes some problems. It looks like the sections are created with different attributes than the original. This causes problems such as sections being writable when the original was read-only, including https://crbug.com/990942 where that causes program crashes. There is some more discussion on that bug report.

Heh, I just missed the revert. @jdenny, feel free to use this as inspiration.

replaced assert with llvm_unreachable

Linking ThinLTO chrome.dll with an undefined symbol:

added comment explaining BitcodeFiles::instances.empty() check

addressed @ruiu's comments

The basic idea here is:

don't process bitcode files in resolveRemainindUndefines

removed redundant check

Ready for review now.

rebase

Simplified after rebasing on top of r366127.

Fix typo pointed out by MaskRay (thanks!)

This was suggested on D64458.

rebased and updated variable naming convention

Just chiming in to say this change makes me very happy. I'm pleasantly surprised by how much easier to read I'm finding the code. Thanks for doing this!

new variable naming style

removed getThinLTOOutputFile identity function

Do we really need to support clang-cl syntax here? Can't the user of -fthinlto-index= use the regular clang driver?

This is similar to r254927 for the clang driver, and reuses the tests from that commit for the backend part. On the frontend, it differs in that clang-cl does not support -x (per the comment in clang/lib/Driver/Driver.cpp line 2068: "// No driver mode exposes -x and /TC or /TP; we don't support mixing them.") Instead of requiring -x ir, the behavior added to clang-cl here is to expect that what would be detected as a native object file based on file extension is really a bitcode file. If it isn't, the backend will cause us to say "error: Invalid bitcode signature", which I think is adequate as a diagnostic.

Thanks for the review!

rebase and back out unnecessary change

clang-format

Added test and reworded commit message.

As an example of what this does, I build Chrome (crrev.com/c/1673234 + crrev.com/c/1654198)'s base_unittests with and without this patch and looked at the sizes of the files generated for ThinLTO (.thinlto.bc, imports files, and native .o files).

Use ModuleToSummariesForIndex to find GUIDs defined or used by the module

Thank you. Given that proceeding with the wrong value will result in either an undefined reference or a reference to what might well be the wrong function at link time, I think making this an error (as you've done) is strictly better.

Can you clarify "which will usually result in a linker error"? E.g. an example of link.exe rejecting the object file or the wrong function being called. The reason I ask is that if we can be sure at compile time that the resulting code will not work or do the wrong thing, I think giving an error is appropriate. But if that isn't the case, we would be rejecting code that cl.exe accepts and we might want to make it a Warning instead.

I think r320996 implements this.

Reverted in r360955.

I also think we should revert this change while we think about the approach. I've been reluctant to do this because I was impressed by how many bytes of output it saves, and I didn't want to lose that just because it doesn't play nice with ThinLTO. However, after some more data gathering and talking to people, I think that:

I put this up for preservation purposes, but I'm not convinced that we should actually go this route, so abandoning.

added symbols and check they are listed

Thanks for helping me think about this and alternative approaches. I'll be withdrawing this for now to get it out of the review queue. If I end up implementing a new approach, I'll put it up as a new diff (with a link to this one for the comments).

• don't hoist allocas if they are not in the same BB as the function that uses them