This is an archive of the discontinued LLVM Phabricator instance.

[clang][driver] Set LTO mode based on input files
AbandonedPublic

Authored by tbaeder on Oct 30 2020, 5:48 AM.

Details

Summary

After setting the LTO mode from the -flto option, look at the input files as well. If any of them is an object file containing LLVM bitcode,
set the LTO mode to either thin or full, depending on the input file.

This makes the following sample work:

clang test.c -c -flto
clang test.o

Which also works when using GCC. Currently this makes non-lld linkers print an error, for example when using ld.bfd:

$ clang test.o
test.o: file not recognized: file format not recognized
clang-10: error: linker command failed with exit code 1 (use -v to see invocation)

Diff Detail

Event Timeline

tbaeder created this revision.Oct 30 2020, 5:48 AM
tbaeder requested review of this revision.Oct 30 2020, 5:48 AM
MaskRay added a comment.EditedNov 11 2020, 11:56 PM

Is the motivation just to avoid -flto or -flto=lto at link time? 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.

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.
  • clangDriver's dependency on LLVMBitReader is a bit odd.

In any case, it is just my opinion that this may not fit well as a driver feature. Hope Teresa can share more authoritative thoughts on this.

clang/lib/Driver/Driver.cpp
1205

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

MaskRay requested changes to this revision.Nov 18 2020, 5:52 PM
This revision now requires changes to proceed.Nov 18 2020, 5:52 PM

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.

clang/lib/Driver/Driver.cpp
1205

Setting the LTO mode from the input files requires the Inputs list, but calling the existing setLTOMode() this late causes problems.

Hey @MaskRay, have you thought about this some more? Or is it too much magic and you'd rather reject it altogether?

MaskRay added a comment.EditedFeb 9 2021, 9:36 AM

Hey @MaskRay, have you thought about this some more? Or is it too much magic and you'd rather reject it altogether?

Yes, I'm afraid it should be rejected. I've seen cases where people want clang -c a.bc to produce an object file instead of enabling LTO.

I don't think .bc files are affected by this patch since they aren't types::TY_Object files, but I might misremember.

tbaeder abandoned this revision.Apr 6 2021, 3:24 AM