[llvm] d9881e6 - [IRMover] Avoid materializing global value that belongs to not-yet-linked module
Yuanfang Chen via llvm-commits
llvm-commits at lists.llvm.org
Wed Oct 7 18:23:21 PDT 2020
Author: Yuanfang Chen
Date: 2020-10-07T18:14:07-07:00
New Revision: d9881e6e27bc7fa882742b13d43bb6d491dfc1ea
URL: https://github.com/llvm/llvm-project/commit/d9881e6e27bc7fa882742b13d43bb6d491dfc1ea
DIFF: https://github.com/llvm/llvm-project/commit/d9881e6e27bc7fa882742b13d43bb6d491dfc1ea.diff
LOG: [IRMover] Avoid materializing global value that belongs to not-yet-linked module
We saw the same assertion failure mentioned here
https://bugs.llvm.org/show_bug.cgi?id=42063 in our internal tests.
The failure happens in the same circumstance as D47898 and D66814 where
uniqueing of DICompositeTypes causes `Mapper::mapValue` to be called on
GlobalValues(`G`) from a not-yet-linked module(`M`). The following type-mapping for
`G` may not complete correctly (fail to unique types etc. depending on the
the complexity of the types) because IRLinker::computeTypeMapping is not done for `M`
in this path.
D47898 and D66814 fixed some type-mapping issue after Mapper::mapValue
is called on `G`. However, it seems it did not handle some complex cases. I
think we should delay linking globals like `G` until its owing module is
linked. In this way, we could save unnecessary type mapping and prune
these corner cases. It is also supposed to reduce the total number of structs
ending up in the combined module.
D47898 is reverted (its test is kept) because it regresses the test case here.
D66814 could also be reverted (the `check-all` looks good). But it looks reasonable
anyway, so I thought I should keep it.
Also tested the patch with clang self-host regularLTO/ThinLTO build, things look
good as well.
Reviewed By: tejohnson
Differential Revision: https://reviews.llvm.org/D87001
Added:
llvm/test/LTO/X86/Inputs/type-mapping-bug4_0.ll
llvm/test/LTO/X86/Inputs/type-mapping-bug4_1.ll
llvm/test/LTO/X86/type-mapping-bug4.ll
Modified:
llvm/lib/Linker/IRMover.cpp
Removed:
################################################################################
diff --git a/llvm/lib/Linker/IRMover.cpp b/llvm/lib/Linker/IRMover.cpp
index 186ddb3d2b81..cb4146a05fdf 100644
--- a/llvm/lib/Linker/IRMover.cpp
+++ b/llvm/lib/Linker/IRMover.cpp
@@ -242,15 +242,6 @@ Type *TypeMapTy::get(Type *Ty, SmallPtrSet<StructType *, 8> &Visited) {
bool IsUniqued = !isa<StructType>(Ty) || cast<StructType>(Ty)->isLiteral();
if (!IsUniqued) {
- StructType *STy = cast<StructType>(Ty);
- // This is actually a type from the destination module, this can be reached
- // when this type is loaded in another module, added to DstStructTypesSet,
- // and then we reach the same type in another module where it has not been
- // added to MappedTypes. (PR37684)
- if (STy->getContext().isODRUniquingDebugTypes() && !STy->isOpaque() &&
- DstStructTypesSet.hasType(STy))
- return *Entry = STy;
-
#ifndef NDEBUG
for (auto &Pair : MappedTypes) {
assert(!(Pair.first != Ty && Pair.second == Ty) &&
@@ -258,7 +249,7 @@ Type *TypeMapTy::get(Type *Ty, SmallPtrSet<StructType *, 8> &Visited) {
}
#endif
- if (!Visited.insert(STy).second) {
+ if (!Visited.insert(cast<StructType>(Ty)).second) {
StructType *DTy = StructType::create(Ty->getContext());
return *Entry = DTy;
}
@@ -579,6 +570,13 @@ Value *IRLinker::materialize(Value *V, bool ForIndirectSymbol) {
if (!SGV)
return nullptr;
+ // When linking a global from other modules than source & dest, skip
+ // materializing it because it would be mapped later when its containing
+ // module is linked. Linking it now would potentially pull in many types that
+ // may not be mapped properly.
+ if (SGV->getParent() != &DstM && SGV->getParent() != SrcM.get())
+ return nullptr;
+
Expected<Constant *> NewProto = linkGlobalValueProto(SGV, ForIndirectSymbol);
if (!NewProto) {
setError(NewProto.takeError());
diff --git a/llvm/test/LTO/X86/Inputs/type-mapping-bug4_0.ll b/llvm/test/LTO/X86/Inputs/type-mapping-bug4_0.ll
new file mode 100644
index 000000000000..9dc490ed1a1e
--- /dev/null
+++ b/llvm/test/LTO/X86/Inputs/type-mapping-bug4_0.ll
@@ -0,0 +1,11 @@
+target datalayout = "e-m:e-i64:64-f80:128-n8:16:32:64-S128"
+target triple = "x86_64-unknown-linux-gnu"
+
+%class.CWBD = type { float }
+%"class.std::_Unique_ptr_base" = type { %class.CWBD* }
+
+%class.CB = type opaque
+
+!llvm.module.flags = !{!0, !1}
+!0 = !{i32 1, !"ThinLTO", i32 0}
+!1 = !{i32 2, !"Debug Info Version", i32 3}
diff --git a/llvm/test/LTO/X86/Inputs/type-mapping-bug4_1.ll b/llvm/test/LTO/X86/Inputs/type-mapping-bug4_1.ll
new file mode 100644
index 000000000000..54fd9d1b4bff
--- /dev/null
+++ b/llvm/test/LTO/X86/Inputs/type-mapping-bug4_1.ll
@@ -0,0 +1,55 @@
+target datalayout = "e-m:e-i64:64-f80:128-n8:16:32:64-S128"
+target triple = "x86_64-unknown-linux-gnu"
+
+%class.CCSM = type opaque
+%class.CWBD = type { float }
+
+%"class.std::_Unique_ptr_base" = type { %class.CWBD* }
+
+%class.CB = type { %"class.std::unique_ptr_base.1" }
+; (stage1.1)
+; %class.std::unique_ptr_base.1(t1.o) is mapped to %class.std::unique_ptr_base(t0.o)
+; %class.CCSM(t1.o) is mapped to %class.CWBD(t0.o)
+%"class.std::unique_ptr_base.1" = type { %class.CCSM* }
+
+; (stage1.2)
+; %class.CCSM(t1.o) -> %class.CWBD(t0.o) mapping of stage1.1 maps this to
+; "declare void @h(%class.CWBD*)"
+declare void @h(%class.CCSM*)
+define void @j() {
+ call void @h(%class.CCSM* undef)
+ ret void
+}
+
+define void @a() {
+ ; Without the fix in D87001 to delay materialization of @d until its module is linked
+ ; (stage1.3)
+ ; mapping `%class.CB* undef` creates the first instance of %class.CB (%class.CB).
+ ; (stage2)
+ ; mapping `!6` starts the stage2, during which second instance of %class.CB (%class.CB.1)
+ ; is created for the mapped @d declaration.
+ ; define void @d(%class.CB.1*)
+ ; After this, %class.CB (t2.o) (aka %class.CB.1) and
+ ; %"class.std::unique_ptr_base.2" (t2.o) are added to DstStructTypesSet.
+ call void @llvm.dbg.value(metadata %class.CB* undef, metadata !6, metadata !DIExpression()), !dbg !4
+ ret void
+}
+
+declare void @llvm.dbg.value(metadata, metadata, metadata)
+
+!llvm.module.flags = !{!0, !1}
+!llvm.dbg.cu = !{!2}
+!0 = !{i32 1, !"ThinLTO", i32 0}
+!1 = !{i32 2, !"Debug Info Version", i32 3}
+!2 = distinct !DICompileUnit(language: DW_LANG_C_plus_plus, file: !3)
+!3 = !DIFile(filename: "f2", directory: "")
+
+!4 = !DILocation(line: 117, column: 34, scope: !7)
+
+; This DICompositeType refers to !5 in type-mapping-bug4.ll
+!5 = !DICompositeType(tag: DW_TAG_structure_type, flags: DIFlagFwdDecl, identifier: "SHARED")
+
+!6 = !DILocalVariable(name: "this", arg: 1, scope: !7, flags: DIFlagArtificial | DIFlagObjectPointer)
+!7 = distinct !DISubprogram(name: "a", type: !8, unit: !2)
+!8 = !DISubroutineType(types: !9)
+!9 = !{null, !5}
diff --git a/llvm/test/LTO/X86/type-mapping-bug4.ll b/llvm/test/LTO/X86/type-mapping-bug4.ll
new file mode 100644
index 000000000000..bdd687cfbefd
--- /dev/null
+++ b/llvm/test/LTO/X86/type-mapping-bug4.ll
@@ -0,0 +1,77 @@
+; RUN: opt -module-summary -o %t0.o %S/Inputs/type-mapping-bug4_0.ll
+; RUN: opt -module-summary -o %t1.o %S/Inputs/type-mapping-bug4_1.ll
+; RUN: opt -module-summary -o %t2.o %s
+; RUN: llvm-lto2 run -save-temps -o %t3 %t0.o %t1.o %t2.o -r %t1.o,a,px -r %t2.o,d,px -r %t1.o,h,x -r %t2.o,h,x -r %t1.o,j,px
+; RUN: llvm-dis < %t3.0.0.preopt.bc | FileCheck %s
+
+; stage0: linking t0.o
+; stage1: linking t1.o
+; stage2: during linking t1.o, mapping @d
+; stage3: linking t2.o
+
+; Stage0 is not described because it is not interesting for the purpose of this test.
+; Stage1 and stage2 are described in type-mapping-bug4_1.ll.
+; Stage3 is described in this file.
+
+; CHECK: %class.CCSM = type opaque
+; CHECK: %class.CB = type { %"class.std::unique_ptr_base.1" }
+; CHECK: %"class.std::unique_ptr_base.1" = type { %class.CCSM* }
+
+; CHECK: define void @j() {
+; CHECK: call void @h(%class.CCSM* undef)
+; CHECK: ret void
+; CHECK: }
+
+; CHECK: declare void @h(%class.CCSM*)
+
+; CHECK: define void @a() {
+; CHECK: call void @llvm.dbg.value(metadata %class.CB* undef, metadata !10, metadata !DIExpression())
+; CHECK: ret void
+; CHECK: }
+
+; CHECK: declare void @llvm.dbg.value(metadata, metadata, metadata) #0
+
+; CHECK: define void @d(%class.CB* %0) {
+; CHECK: %2 = getelementptr inbounds %class.CB, %class.CB* undef, i64 0, i32 0, i32 0
+; CHECK: ret void
+; CHECK: }
+
+target datalayout = "e-m:e-i64:64-f80:128-n8:16:32:64-S128"
+target triple = "x86_64-unknown-linux-gnu"
+
+; (stage3) Remapping this type returns itself due to D47898 and stage1.3
+%class.CB = type { %"class.std::unique_ptr_base.2" }
+
+; (stage3) Remapping this type returns itself due to D47898 and stage2
+%"class.std::unique_ptr_base.2" = type { %class.CCSM* }
+
+%class.CCSM = type opaque
+
+; (stage3) computeTypeMapping add the mapping %class.CCSM -> %class.CWBD due to stage1.2
+declare void @h(%class.CCSM*)
+
+define void @d(%class.CB*) {
+ ; Without the fix in D87001 to delay materialization of @d until its module is linked
+ ; (stage3)
+ ; * SourceElementType of getelementptr is remapped to itself.
+ ; * ResultElementType of getelementptr is incorrectly remapped to %class.CWBD*.
+ ; Its type should be %class.CCSM*.
+ %2 = getelementptr inbounds %class.CB, %class.CB* undef, i64 0, i32 0, i32 0
+ ret void
+}
+
+!llvm.dbg.cu = !{!2}
+!llvm.module.flags = !{!0, !1}
+!0 = !{i32 1, !"ThinLTO", i32 0}
+!1 = !{i32 2, !"Debug Info Version", i32 3}
+!2 = distinct !DICompileUnit(language: DW_LANG_C_plus_plus, file: !3, retainedTypes: !4)
+!3 = !DIFile(filename: "f1", directory: "")
+!4 = !{!5}
+
+; This DICompositeType is referenced by !5 in Inputs/type-mapping-bug4_1.ll
+; causing the function type in !7 to be added to its module.
+!5 = !DICompositeType(tag: DW_TAG_structure_type, templateParams: !6, identifier: "SHARED")
+!6 = !{!7}
+
+; The reference to d and %class.CB that gets loaded into %t1.o
+!7 = !DITemplateValueParameter(value: void (%class.CB*)* @d)
More information about the llvm-commits
mailing list