[ThinLTO] Allow indexing to request backend to ignore the module
ClosedPublic

Authored by vitalybuka on Tue, Feb 6, 5:17 PM.

Details

Summary

Gold plugin does not add pass to ThinLTO modules without useful symbols.
In this case ThinLTO can't create corresponding index file and some features, like CFI,
cannot be processes by backed correctly without index.
Given that we don't need the backed output we can request it to avoid
processing the module. This is implemented by this patch using new "SkipModule"
flag.

Diff Detail

Repository
rC Clang
vitalybuka created this revision.Tue, Feb 6, 5:17 PM

Empty ThinLTOIndexFile signals that we don't need this module during
linking.

Not the only case actually. We now also pass an empty index file when we want to compile the bitcode down to object without applying any LTO optimization (there are a few cases where we decide we want to turn off LTO optimizations for some links), and this is currently relying on being able to pass /dev/null for the index file that would be broken by this change.

So we should not run ThinLTO backend even if it contains the
ThinLTO module. Backend may fail because of lack of necessary
information which should be provided by ThinLTOIndex.

This shouldn't happen - are you seeing cases where we fail? After loadModule() is called, EmitBackendOutput() is called which passes /*IgnoreEmptyThinLTOIndexFile*/true to getModuleSummaryIndexForFile, which would cause it to return nullptr if the index file is empty. Back in EmitBackendOutput(), if the combined index is null we will skip ThinLTO compilation and fall back to normal compilation.

Empty ThinLTOIndexFile signals that we don't need this module during
linking.

Not the only case actually. We now also pass an empty index file when we want to compile the bitcode down to object without applying any LTO optimization (there are a few cases where we decide we want to turn off LTO optimizations for some links), and this is currently relying on being able to pass /dev/null for the index file that would be broken by this change.

I'd expect this should be done by indexing and content is already in the merged object file.
Not sure how to reproduce this. I've build some large targets and I never seen this.

So we should not run ThinLTO backend even if it contains the
ThinLTO module. Backend may fail because of lack of necessary
information which should be provided by ThinLTOIndex.

This shouldn't happen - are you seeing cases where we fail? After loadModule() is called, EmitBackendOutput() is called which passes /*IgnoreEmptyThinLTOIndexFile*/true to getModuleSummaryIndexForFile, which would cause it to return nullptr if the index file is empty. Back in EmitBackendOutput(), if the combined index is null we will skip ThinLTO compilation and fall back to normal compilation.

I don't see for regular compilation, but I see for for CFI. Backend will not be able to process llvm.type.test without TypeIdMap from index and it will crash in "Instruction Select"

Empty ThinLTOIndexFile signals that we don't need this module during
linking.

Not the only case actually. We now also pass an empty index file when we want to compile the bitcode down to object without applying any LTO optimization (there are a few cases where we decide we want to turn off LTO optimizations for some links), and this is currently relying on being able to pass /dev/null for the index file that would be broken by this change.

I'd expect this should be done by indexing and content is already in the merged object file.
Not sure how to reproduce this. I've build some large targets and I never seen this.

At least with gold I don't see how this possible. I see that thinlto.bc can be empty only
if getSymbolsAndView returns nullptr or if LTOInfo for input object was false.
Former means that we don't need this object and so I created this patch.
For latter we already going to do the same anyway: D42680

Empty ThinLTOIndexFile signals that we don't need this module during
linking.

Not the only case actually. We now also pass an empty index file when we want to compile the bitcode down to object without applying any LTO optimization (there are a few cases where we decide we want to turn off LTO optimizations for some links), and this is currently relying on being able to pass /dev/null for the index file that would be broken by this change.

I'd expect this should be done by indexing and content is already in the merged object file.
Not sure how to reproduce this. I've build some large targets and I never seen this.

This is done in bazel, not in gold. See below.

So we should not run ThinLTO backend even if it contains the
ThinLTO module. Backend may fail because of lack of necessary
information which should be provided by ThinLTOIndex.

This shouldn't happen - are you seeing cases where we fail? After loadModule() is called, EmitBackendOutput() is called which passes /*IgnoreEmptyThinLTOIndexFile*/true to getModuleSummaryIndexForFile, which would cause it to return nullptr if the index file is empty. Back in EmitBackendOutput(), if the combined index is null we will skip ThinLTO compilation and fall back to normal compilation.

I don't see for regular compilation, but I see for for CFI. Backend will not be able to process llvm.type.test without TypeIdMap from index and it will crash in "Instruction Select"

@pcc and I already discussed this a little bit in the context of bazel. I will point you to that discussion separately.

vitalybuka planned changes to this revision.Tue, Feb 13, 10:30 AM

Don't use empty index file

vitalybuka retitled this revision from [ThinLTO] Ignore object files with empty ThinLTO index to [ThinLTO] Allow indexing to request backend to ignore the module.Thu, Feb 15, 12:22 AM
vitalybuka edited the summary of this revision. (Show Details)
tejohnson added inline comments.Thu, Feb 15, 9:40 PM
clang/lib/CodeGen/BackendUtil.cpp
1176 ↗(On Diff #134385)

s/generated/generate an/

clang/test/CodeGen/thinlto-distributed-backend-skip.ll
3 ↗(On Diff #134385)

s/backed/backend/

llvm/include/llvm/IR/ModuleSummaryIndex.h
685 ↗(On Diff #134385)

Need to document clearly that this only applies to the distributed index case, and what it means to "skip" the module (i.e. skip the backend compilation and generate an empty module instead).

Maybe rename to something like SkipModuleDistributedBackend or something else very explicit to avoid confusion?

llvm/tools/gold/gold-plugin.cpp
819 ↗(On Diff #134385)

Rename Skip to SkipModule to match the above comment documenting parameter

890 ↗(On Diff #134385)

Think this should be "else if (options::thinlto_index_only) {"
i.e. we don't need/want to write these when we're in single process mode.

892 ↗(On Diff #134385)

Document constant parameters here and in other places. e.g. "/* SkipModule= */ true"

vitalybuka marked 6 inline comments as done.

addressed comments

This revision is now accepted and ready to land.Fri, Feb 16, 11:29 AM
This revision was automatically updated to reflect the committed changes.