Skip to content

Commit d0b1f30

Browse files
teresajohnsonteresajohnson
committedFeb 14, 2019
[ThinLTO] Detect partially split modules during the thin link
Summary: The changes to disable LTO unit splitting by default (r350949) and detect inconsistently split LTO units (r350948) are causing some crashes when the inconsistency is detected in multiple threads simultaneously. Fix that by having the code always look for the inconsistently split LTO units during the thin link, by checking for the presence of type tests recorded in the summaries. Modify test added in r350948 to remove single threading required to fix a bot failure due to this issue (and some debugging options added in the process of diagnosing it). Reviewers: pcc Subscribers: mehdi_amini, inglorion, eraman, steven_wu, dexonsmith, llvm-commits Tags: #llvm Differential Revision: https://reviews.llvm.org/D57561 llvm-svn: 354062
1 parent 8e918d6 commit d0b1f30

File tree

5 files changed

+79
-32
lines changed

5 files changed

+79
-32
lines changed
 

‎llvm/include/llvm/LTO/LTO.h

+2
Original file line numberDiff line numberDiff line change
@@ -398,6 +398,8 @@ class LTO {
398398
Error runRegularLTO(AddStreamFn AddStream);
399399
Error runThinLTO(AddStreamFn AddStream, NativeObjectCache Cache);
400400

401+
Error checkPartiallySplit();
402+
401403
mutable bool CalledGetMaxTasks = false;
402404

403405
// Use Optional to distinguish false from not yet initialized.

‎llvm/lib/LTO/LTO.cpp

+51-5
Original file line numberDiff line numberDiff line change
@@ -20,6 +20,7 @@
2020
#include "llvm/Config/llvm-config.h"
2121
#include "llvm/IR/AutoUpgrade.h"
2222
#include "llvm/IR/DiagnosticPrinter.h"
23+
#include "llvm/IR/Intrinsics.h"
2324
#include "llvm/IR/LegacyPassManager.h"
2425
#include "llvm/IR/Mangler.h"
2526
#include "llvm/IR/Metadata.h"
@@ -808,6 +809,45 @@ unsigned LTO::getMaxTasks() const {
808809
return RegularLTO.ParallelCodeGenParallelismLevel + ThinLTO.ModuleMap.size();
809810
}
810811

812+
// If only some of the modules were split, we cannot correctly handle
813+
// code that contains type tests or type checked loads.
814+
Error LTO::checkPartiallySplit() {
815+
if (!ThinLTO.CombinedIndex.partiallySplitLTOUnits())
816+
return Error::success();
817+
818+
Function *TypeTestFunc = RegularLTO.CombinedModule->getFunction(
819+
Intrinsic::getName(Intrinsic::type_test));
820+
Function *TypeCheckedLoadFunc = RegularLTO.CombinedModule->getFunction(
821+
Intrinsic::getName(Intrinsic::type_checked_load));
822+
823+
// First check if there are type tests / type checked loads in the
824+
// merged regular LTO module IR.
825+
if ((TypeTestFunc && !TypeTestFunc->use_empty()) ||
826+
(TypeCheckedLoadFunc && !TypeCheckedLoadFunc->use_empty()))
827+
return make_error<StringError>(
828+
"inconsistent LTO Unit splitting (recompile with -fsplit-lto-unit)",
829+
inconvertibleErrorCode());
830+
831+
// Otherwise check if there are any recorded in the combined summary from the
832+
// ThinLTO modules.
833+
for (auto &P : ThinLTO.CombinedIndex) {
834+
for (auto &S : P.second.SummaryList) {
835+
auto *FS = dyn_cast<FunctionSummary>(S.get());
836+
if (!FS)
837+
continue;
838+
if (!FS->type_test_assume_vcalls().empty() ||
839+
!FS->type_checked_load_vcalls().empty() ||
840+
!FS->type_test_assume_const_vcalls().empty() ||
841+
!FS->type_checked_load_const_vcalls().empty() ||
842+
!FS->type_tests().empty())
843+
return make_error<StringError>(
844+
"inconsistent LTO Unit splitting (recompile with -fsplit-lto-unit)",
845+
inconvertibleErrorCode());
846+
}
847+
}
848+
return Error::success();
849+
}
850+
811851
Error LTO::run(AddStreamFn AddStream, NativeObjectCache Cache) {
812852
// Compute "dead" symbols, we don't want to import/export these!
813853
DenseSet<GlobalValue::GUID> GUIDPreservedSymbols;
@@ -850,6 +890,17 @@ Error LTO::run(AddStreamFn AddStream, NativeObjectCache Cache) {
850890
StatsFile->keep();
851891
}
852892

893+
// Finalize linking of regular LTO modules containing summaries now that
894+
// we have computed liveness information.
895+
for (auto &M : RegularLTO.ModsWithSummaries)
896+
if (Error Err = linkRegularLTO(std::move(M),
897+
/*LivenessFromIndex=*/true))
898+
return Err;
899+
900+
// Ensure we don't have inconsistently split LTO units with type tests.
901+
if (Error Err = checkPartiallySplit())
902+
return Err;
903+
853904
Error Result = runRegularLTO(AddStream);
854905
if (!Result)
855906
Result = runThinLTO(AddStream, Cache);
@@ -861,11 +912,6 @@ Error LTO::run(AddStreamFn AddStream, NativeObjectCache Cache) {
861912
}
862913

863914
Error LTO::runRegularLTO(AddStreamFn AddStream) {
864-
for (auto &M : RegularLTO.ModsWithSummaries)
865-
if (Error Err = linkRegularLTO(std::move(M),
866-
/*LivenessFromIndex=*/true))
867-
return Err;
868-
869915
// Make sure commons have the right size/alignment: we kept the largest from
870916
// all the prevailing when adding the inputs, and we apply it here.
871917
const DataLayout &DL = RegularLTO.CombinedModule->getDataLayout();

‎llvm/lib/Transforms/IPO/LowerTypeTests.cpp

+8-7
Original file line numberDiff line numberDiff line change
@@ -1692,6 +1692,14 @@ void LowerTypeTestsModule::replaceDirectCalls(Value *Old, Value *New) {
16921692
}
16931693

16941694
bool LowerTypeTestsModule::lower() {
1695+
// If only some of the modules were split, we cannot correctly perform
1696+
// this transformation. We already checked for the presense of type tests
1697+
// with partially split modules during the thin link, and would have emitted
1698+
// an error if any were found, so here we can simply return.
1699+
if ((ExportSummary && ExportSummary->partiallySplitLTOUnits()) ||
1700+
(ImportSummary && ImportSummary->partiallySplitLTOUnits()))
1701+
return false;
1702+
16951703
Function *TypeTestFunc =
16961704
M.getFunction(Intrinsic::getName(Intrinsic::type_test));
16971705
Function *ICallBranchFunnelFunc =
@@ -1701,13 +1709,6 @@ bool LowerTypeTestsModule::lower() {
17011709
!ExportSummary && !ImportSummary)
17021710
return false;
17031711

1704-
// If only some of the modules were split, we cannot correctly handle
1705-
// code that contains type tests.
1706-
if (TypeTestFunc && !TypeTestFunc->use_empty() &&
1707-
((ExportSummary && ExportSummary->partiallySplitLTOUnits()) ||
1708-
(ImportSummary && ImportSummary->partiallySplitLTOUnits())))
1709-
report_fatal_error("inconsistent LTO Unit splitting with llvm.type.test");
1710-
17111712
if (ImportSummary) {
17121713
if (TypeTestFunc) {
17131714
for (auto UI = TypeTestFunc->use_begin(), UE = TypeTestFunc->use_end();

‎llvm/lib/Transforms/IPO/WholeProgramDevirt.cpp

+8-11
Original file line numberDiff line numberDiff line change
@@ -1563,23 +1563,20 @@ void DevirtModule::removeRedundantTypeTests() {
15631563
}
15641564

15651565
bool DevirtModule::run() {
1566+
// If only some of the modules were split, we cannot correctly perform
1567+
// this transformation. We already checked for the presense of type tests
1568+
// with partially split modules during the thin link, and would have emitted
1569+
// an error if any were found, so here we can simply return.
1570+
if ((ExportSummary && ExportSummary->partiallySplitLTOUnits()) ||
1571+
(ImportSummary && ImportSummary->partiallySplitLTOUnits()))
1572+
return false;
1573+
15661574
Function *TypeTestFunc =
15671575
M.getFunction(Intrinsic::getName(Intrinsic::type_test));
15681576
Function *TypeCheckedLoadFunc =
15691577
M.getFunction(Intrinsic::getName(Intrinsic::type_checked_load));
15701578
Function *AssumeFunc = M.getFunction(Intrinsic::getName(Intrinsic::assume));
15711579

1572-
// If only some of the modules were split, we cannot correctly handle
1573-
// code that contains type tests or type checked loads.
1574-
if ((ExportSummary && ExportSummary->partiallySplitLTOUnits()) ||
1575-
(ImportSummary && ImportSummary->partiallySplitLTOUnits())) {
1576-
if ((TypeTestFunc && !TypeTestFunc->use_empty()) ||
1577-
(TypeCheckedLoadFunc && !TypeCheckedLoadFunc->use_empty()))
1578-
report_fatal_error("inconsistent LTO Unit splitting with llvm.type.test "
1579-
"or llvm.type.checked.load");
1580-
return false;
1581-
}
1582-
15831580
// Normally if there are no users of the devirtualization intrinsics in the
15841581
// module, this pass has nothing to do. But if we are exporting, we also need
15851582
// to handle any users that appear only in the function summaries.

‎llvm/test/ThinLTO/X86/cfi-devirt.ll

+10-9
Original file line numberDiff line numberDiff line change
@@ -20,8 +20,8 @@
2020
; RUN: -r=%t.o,_ZN1B1fEi, \
2121
; RUN: -r=%t.o,_ZN1C1fEi, \
2222
; RUN: -r=%t.o,_ZTV1B,px \
23-
; RUN: -r=%t.o,_ZTV1C,px 2>&1 | FileCheck %s --check-prefix=REMARK -dump-input=always
24-
; RUN: llvm-dis %t3.1.4.opt.bc -o - | FileCheck %s --check-prefix=CHECK-IR -dump-input=always
23+
; RUN: -r=%t.o,_ZTV1C,px 2>&1 | FileCheck %s --check-prefix=REMARK
24+
; RUN: llvm-dis %t3.1.4.opt.bc -o - | FileCheck %s --check-prefix=CHECK-IR
2525

2626
; New PM
2727
; FIXME: Fix machine verifier issues and remove -verify-machineinstrs=0. PR39436.
@@ -39,17 +39,18 @@
3939
; RUN: -r=%t.o,_ZN1B1fEi, \
4040
; RUN: -r=%t.o,_ZN1C1fEi, \
4141
; RUN: -r=%t.o,_ZTV1B,px \
42-
; RUN: -r=%t.o,_ZTV1C,px 2>&1 | FileCheck %s --check-prefix=REMARK -dump-input=always
43-
; RUN: llvm-dis %t3.1.4.opt.bc -o - | FileCheck %s --check-prefix=CHECK-IR -dump-input=always
42+
; RUN: -r=%t.o,_ZTV1C,px 2>&1 | FileCheck %s --check-prefix=REMARK
43+
; RUN: llvm-dis %t3.1.4.opt.bc -o - | FileCheck %s --check-prefix=CHECK-IR
4444

4545
; REMARK: single-impl: devirtualized a call to _ZN1A1nEi
4646

4747
; Next check that we emit an error when trying to LTO link this module
4848
; containing an llvm.type.checked.load (with a split LTO Unit) with one
49-
; that does not have a split LTO Unit.
49+
; that does not have a split LTO Unit. Use -thinlto-distributed-indexes
50+
; to ensure it is being caught in the thin link.
5051
; RUN: opt -thinlto-bc -o %t2.o %S/Inputs/empty.ll
51-
; RUN: not llvm-lto2 run %t.o %t2.o -save-temps -pass-remarks=. \
52-
; RUN: -verify-machineinstrs=0 -thinlto-threads=1 \
52+
; RUN: not llvm-lto2 run %t.o %t2.o -thinlto-distributed-indexes \
53+
; RUN: -verify-machineinstrs=0 \
5354
; RUN: -o %t3 \
5455
; RUN: -r=%t.o,test,px \
5556
; RUN: -r=%t.o,_ZN1A1nEi,p \
@@ -62,8 +63,8 @@
6263
; RUN: -r=%t.o,_ZN1B1fEi, \
6364
; RUN: -r=%t.o,_ZN1C1fEi, \
6465
; RUN: -r=%t.o,_ZTV1B,px \
65-
; RUN: -r=%t.o,_ZTV1C,px 2>&1 | FileCheck %s --check-prefix=ERROR -dump-input=always
66-
; ERROR: LLVM ERROR: inconsistent LTO Unit splitting with llvm.type.test or llvm.type.checked.load
66+
; RUN: -r=%t.o,_ZTV1C,px 2>&1 | FileCheck %s --check-prefix=ERROR
67+
; ERROR: failed: inconsistent LTO Unit splitting (recompile with -fsplit-lto-unit)
6768

6869
target datalayout = "e-m:e-i64:64-f80:128-n8:16:32:64-S128"
6970
target triple = "x86_64-grtev4-linux-gnu"

0 commit comments

Comments
 (0)
Please sign in to comment.