This is an archive of the discontinued LLVM Phabricator instance.

[ThinLTO] Ignore object files with no ThinLTO modules if -fthinlto-index= is set
ClosedPublic

Authored by vitalybuka on Jan 30 2018, 2:12 AM.

Details

Summary

ThinLTO compilation may decide not to split module and keep at as regular LTO.
In this can this module already processed during indexing and already a part of
merged object file. So here we can just skip it.

Event Timeline

vitalybuka created this revision.Jan 30 2018, 2:12 AM
tejohnson accepted this revision.Jan 30 2018, 9:06 AM

LGTM (minor comment fix in test needed)

clang/test/CodeGen/thinlto_backend.ll
23

Comment line length too long, and sentence isn't complete.

This revision is now accepted and ready to land.Jan 30 2018, 9:06 AM
pcc requested changes to this revision.Jan 30 2018, 9:14 AM

This doesn't seem right to me. In a mixed full/thin LTO link the full LTO module would be compiled during the indexing phase. We don't want to compile it again in the backend as it could lead at best to duplicate symbol errors and at worst to miscompiles of llvm.type.* intrinsic calls.

This revision now requires changes to proceed.Jan 30 2018, 9:14 AM
In D42680#991938, @pcc wrote:

This doesn't seem right to me. In a mixed full/thin LTO link the full LTO module would be compiled during the indexing phase. We don't want to compile it again in the backend as it could lead at best to duplicate symbol errors and at worst to miscompiles of llvm.type.* intrinsic calls.

Good point. The build system will presumably expect an output file to be generated, so probably just compile an "empty" Module in this case?

vitalybuka updated this revision to Diff 132701.Feb 2 2018, 4:22 PM

skip full LTO objects

vitalybuka retitled this revision from [ThinLTO] Ignore -fthinlto-index= for non-ThinLTO files to [ThinLTO] Ignore object files with no ThinLTO modules if -fthinlto-index= is set.Feb 2 2018, 4:23 PM
vitalybuka edited the summary of this revision. (Show Details)
vitalybuka updated this revision to Diff 132702.Feb 2 2018, 4:24 PM
vitalybuka marked an inline comment as done.

comment

This looks ok to me, assuming it produces the empty output file as I expect it should. Please wait for pcc to take a look as well.

clang/lib/CodeGen/CodeGenAction.cpp
959

It looks like this should result in an empty .o file being produced, but please confirm in the test (otherwise the build system may complain that expected output not produced).

clang/test/CodeGen/thinlto_backend.ll
24

comment isn't complete (ends in "which").

26

Check that empty $t4.o is produced.

vitalybuka marked 3 inline comments as done.

empty file test

vitalybuka updated this revision to Diff 133111.Feb 6 2018, 4:14 PM

Create valid empty object file

vitalybuka updated this revision to Diff 133116.Feb 6 2018, 5:15 PM

set triple

fix rebase issues introduced in the last patch

@pcc Could you please take a look?

pcc accepted this revision.Feb 16 2018, 1:08 PM

LGTM

clang/test/CodeGen/thinlto_backend.ll
27

llvm-nm %t4.o | count 0

This revision is now accepted and ready to land.Feb 16 2018, 1:08 PM
vitalybuka marked an inline comment as done.

use count 0

This revision was automatically updated to reflect the committed changes.
This revision was automatically updated to reflect the committed changes.