[PATCH] D90457: [clang][driver] Set LTO mode based on input files

Fri Nov 27 01:56:02 PST 2020

tbaeder added a comment.

In D90457#2390519 <https://reviews.llvm.org/D90457#2390519>, @MaskRay wrote:

> Is the motivation just to avoid -flto or -flto=lto at link time?

Essentially, yes. Lots of projects seem to depend on this behavior of GCC. I.e. the pass -flto when compiling but not when linking.

> I am afraid that the advantage probably is not large enough to justify the potentially costly object file parsing in the driver. Before this, as I understand it, object files are opaque to the driver. The driver just passes all file names to the linker. With this change, every object file will be opened and `llvm::getBitcodeFileContents` will be called on every object file.

I thought about the potential performance penalties as well. All of this only matters if -flto is really missing of course, otherwise the driver is not going to do any more work than it did before.

> Other thoughts include:
>
> - An LTO link can mix regular and thin LTO bitcode files. I am not very familiar with the thinlto internals so I don't know whether it is appropriate to stop after we immediately see a full LTO bitcode file.
> - This provides a convenience method in the most simplest link (no other LTO related option). If you specify any other LTO related option (e.g --thinlto-index-only, --thinlto-cache-dir), the save of a `-flto` option appears to give too small of a value with an extra parsing.

Right, the purpose of this is not to save typing a "-flto", but to make linking in the simplest cast just work.

================
Comment at: clang/lib/Driver/Driver.cpp:1205

+  setLTOModeFromInputFiles(&Inputs);
+
----------------
MaskRay wrote:
> On line 1133, there is a similar setLTOMode. If we are going to add the logic, probably consider unifying the logic. However, I am concerned with the cost, see my main comment
Setting the LTO mode from the input files requires the Inputs list, but calling the existing setLTOMode() this late causes problems.

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D90457/new/

https://reviews.llvm.org/D90457