[IRMover] Avoid materializing global value that belongs to not-yet-linked module

[ychen]

Summary

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.

[ychen]

• ready for review

[tejohnson]

I need to think through this some more, but a couple questions:

Was the clang self host that you tried regular or Thin LTO? If regular please also try ThinLTO.

Do you have a test case showing the failure you got? I think it will be important to add if at all possible.

[ychen]

In D87001#2252711, @tejohnson wrote:

I need to think through this some more, but a couple questions:

Was the clang self host that you tried regular or Thin LTO? If regular please also try ThinLTO.

I was testing the patch with regular LTO. Will try ThinLTO also.

Do you have a test case showing the failure you got? I think it will be important to add if at all possible.

The original test case is pretty big. I'll try harder to get it reduced.

[ychen]

In D87001#2252711, @tejohnson wrote:

I need to think through this some more, but a couple questions:

Was the clang self host that you tried regular or Thin LTO? If regular please also try ThinLTO.

Do you have a test case showing the failure you got? I think it will be important to add if at all possible.

Before I could get a reduced test case, could you please try the reproducer attached in PR42063? It seems to have dependencies on fuchsia.

[ychen]

• update test to demonstrate the bug
• actually the bug could be fixed by reverting D47898
• ThinLTO self-host Clang looks good.

[ychen]

gentle ping...

[ychen]

ping..

[tejohnson]

It sounds like even the test case that was added for D47898 passes when that fix is reverted? Can you also try the original test case from https://bugs.llvm.org/show_bug.cgi?id=37684, which is just a little bit different? Since this handling has been problematic, I'd like to understand what changed to make that older fix unnecessary.

[ychen]

In D87001#2317777, @tejohnson wrote:

It sounds like even the test case that was added for D47898 passes when that fix is reverted?

Sorry, that's not what I meant. I meant to say that D47898 regressed the test case in this patch. (Ps, I've verified D47898 does fix its own test case).

[tejohnson]

In D87001#2317990, @ychen wrote:

In D87001#2317777, @tejohnson wrote:

It sounds like even the test case that was added for D47898 passes when that fix is reverted?

Sorry, that's not what I meant. I meant to say that D47898 regressed the test case in this patch. (Ps, I've verified D47898 does fix its own test case).

Ok got it. I should have re-read the original summary again! You are essentially reverting D47898 and replacing with a more comprehensive fix. Thanks for adding a test case. I added some suggestions to the test, mostly just comment clarity.

llvm/test/LTO/X86/Inputs/type-mapping-bug4_1.ll

12

I assume t1 refers to %t1.o, etc? Would be clearer to spell that out here and elsewhere.

25

Since in the future it won't be obvious what "this" fix is, suggest making this something like "Without the fix in D87001 to delay materialization of @d until its module is linked".

llvm/test/LTO/X86/type-mapping-bug4.ll

5

I assume this fails without this fix? Would also be good to add some checking of the resulting linked code.

15

The steps vs stages is confusing to me. Suggest noting above where you describe the stages where the steps fit. Also suggest mentioning where these steps are described, i.e. "due to step3.1 described in type-mapping-bug4_1.ll". Or maybe if you mention above where listing stages which step is included in which stage you could say there where each step is described.

27

Similar comment to the one in type-mapping-bug_1.ll - explicitly specify what the fix is.

[ychen]

• fix comments, especially remove references to "step"s. The sequence of events are described in "stage"s instead.

[ychen]

• Check for preopt IR dump

[tejohnson]

LGTM with a minor comment nit and some clarification needed about stage 2 vs stage 3 in the description as noted below.

llvm/test/LTO/X86/Inputs/type-mapping-bug4_1.ll

30

This mentions stage2, seems inconsistent with stage3.2?

llvm/test/LTO/X86/type-mapping-bug4.ll

11

nit "t2.o" here and t1.o on the line above.

[ychen]

• fix more comment

llvm/lib/Linker/IRMover.cpp

Type *TypeMapTy::get(Type *Ty, SmallPtrSet<StructType *, 8> &Visited) {
   Type **Entry = &MappedTypes[Ty];
   if (*Entry)
     return *Entry;
 
   // These are types that LLVM itself will unique.
   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) &&
              "mapping to a source type");
     }
 #endif
 
-    if (!Visited.insert(STy).second) {
+    if (!Visited.insert(cast<StructType>(Ty)).second) {
       StructType *DTy = StructType::create(Ty->getContext());
       return *Entry = DTy;
     }
   }
 
   // If this is not a recursive type, then just map all of the elements and
   // then rebuild the type from inside out.
   SmallVector<Type *, 4> ElementTypes;
Value *LocalValueMaterializer::materialize(Value *SGV) {
   return TheIRLinker.materialize(SGV, true);
 }
 
 Value *IRLinker::materialize(Value *V, bool ForIndirectSymbol) {
   auto *SGV = dyn_cast<GlobalValue>(V);
   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());
     return nullptr;
   }
   if (!*NewProto)
     return nullptr;
 

llvm/test/LTO/X86/Inputs/type-mapping-bug4_0.ll

+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}

llvm/test/LTO/X86/Inputs/type-mapping-bug4_1.ll

+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)
+;   %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* }
+
+; (stage2)
+;   %class.CCSM(t1.o) -> %class.CWBD(t0.o) mapping of stage1 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
+  ; (stage3.1)
+  ;   mapping `%class.CB* undef` creates the first instance of %class.CB (%class.CB).
+  ; (stage3.2)
+  ;   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) (aka %class.CB.1) and
+  ;   %"class.std::unique_ptr_base.2" (t2) 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}

llvm/test/LTO/X86/type-mapping-bug4.ll

+; 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 -o %ta %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
+
+; stage0: linking t0.o
+; stage1: linking t1.o
+; stage2: during linking t1, mapping @d
+; stage3: linking t2
+; All stages except stage0 are described in type-mapping-bug4_1.ll. Stage0 is
+; not described because it is not interesting for the purpose of this test.
+
+target datalayout = "e-m:e-i64:64-f80:128-n8:16:32:64-S128"
+target triple = "x86_64-unknown-linux-gnu"
+
+; Remapping this type returns itself due to D47898 due to stage3.1
+%class.CB = type { %"class.std::unique_ptr_base.2" }
+
+; Remapping this type returns itself due to D47898 due to stage3.2
+%"class.std::unique_ptr_base.2" = type { %class.CCSM* }
+
+%class.CCSM = type opaque
+
+; computeTypeMapping add the mapping %class.CCSM -> %class.CWBD due to stage2
+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
+  ; * SourceElementType %class.CB* is remapped to itself.
+  ; * ResultElementType %class.CB* is remapped to %class.CWBD* but type of %2
+  ;   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)