[ThinLTO] Do not duplicate import a function that is actually defined in the current module #110064

[huangjd]

Trying to land #110064 again after fixing test case

[llvmbot]

@llvm/pr-subscribers-llvm-transforms

@llvm/pr-subscribers-lto

Author: William Junda Huang (huangjd)

Changes

Trying to land #110064 again after fixing test case

---

Full diff: https://github.com/llvm/llvm-project/pull/111933.diff

3 Files Affected:

• (modified) llvm/lib/Linker/IRMover.cpp (+5-1)
• (added) llvm/test/ThinLTO/X86/Inputs/ditemplatevalueparameter-remap.ll (+29)
• (added) llvm/test/ThinLTO/X86/ditemplatevalueparameter-remap.ll (+92)

diff --git a/llvm/lib/Linker/IRMover.cpp b/llvm/lib/Linker/IRMover.cpp
index 3a6c2678cd157f..5067fbff2e277b 100644
--- a/llvm/lib/Linker/IRMover.cpp
+++ b/llvm/lib/Linker/IRMover.cpp
@@ -595,11 +595,15 @@ Value *IRLinker::materialize(Value *V, bool ForIndirectSymbol) {
   if (!SGV)
     return nullptr;
 
+  // If SGV is from dest, it was already materialized when dest was loaded.
+  if (SGV->getParent() == &DstM)
+    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())
+  if (SGV->getParent() != SrcM.get())
     return nullptr;
 
   Expected<Constant *> NewProto = linkGlobalValueProto(SGV, ForIndirectSymbol);
diff --git a/llvm/test/ThinLTO/X86/Inputs/ditemplatevalueparameter-remap.ll b/llvm/test/ThinLTO/X86/Inputs/ditemplatevalueparameter-remap.ll
new file mode 100644
index 00000000000000..be93160b943397
--- /dev/null
+++ b/llvm/test/ThinLTO/X86/Inputs/ditemplatevalueparameter-remap.ll
@@ -0,0 +1,29 @@
+target datalayout = "e-m:e-p270:32:32-p271:32:32-p272:64:64-i64:64-i128:128-f80:128-n8:16:32:64-S128"
+target triple = "x86_64-unknown-linux-gnu"
+
+define void @_Z8thinlto1v() unnamed_addr {
+  %3 = alloca i64, align 4
+    #dbg_declare(ptr %3, !14, !DIExpression(), !15)
+  ret void
+}
+
+!llvm.dbg.cu = !{!0}
+!llvm.module.flags = !{!2, !3, !4, !5}
+
+!0 = distinct !DICompileUnit(language: DW_LANG_C_plus_plus_14, file: !1, producer: "clang", isOptimized: true, runtimeVersion: 0, emissionKind: FullDebug, splitDebugInlining: false, nameTableKind: None)
+!1 = !DIFile(filename: "B.cpp", directory: ".")
+!2 = !{i32 7, !"Dwarf Version", i32 4}
+!3 = !{i32 2, !"Debug Info Version", i32 3}
+!4 = !{i32 1, !"wchar_size", i32 4}
+!5 = !{i32 8, !"PIC Level", i32 2}
+!10 = distinct !DISubprogram(name: "thinlto1", linkageName: "_Z8thinlto1v", scope: !11, file: !11, line: 8, type: !12, scopeLine: 8, flags: DIFlagPrototyped | DIFlagAllCallsDescribed, spFlags: DISPFlagDefinition | DISPFlagOptimized, unit: !0)
+!11 = !DIFile(filename: "b.cpp", directory: ".")
+!12 = !DISubroutineType(types: !13)
+!13 = !{null}
+!14 = !DILocalVariable(name: "a", arg: 1, scope: !10, file: !11, line: 18, type: !16)
+!15 = !DILocation(line: 18, column: 19, scope: !10)
+!16 = distinct !DICompositeType(tag: DW_TAG_structure_type, name: "S<&func1>", file: !11, line: 2, size: 8, flags: DIFlagTypePassByValue, elements: !17, templateParams: !18, identifier: "_ZTS1SIXadL_Z5func1vEEE")
+!17 = !{}
+!18 = !{!19}
+!19 = !DITemplateValueParameter(name: "Func", type: !20, value: ptr undef)
+!20 = !DIDerivedType(tag: DW_TAG_pointer_type, baseType: !12, size: 64)
diff --git a/llvm/test/ThinLTO/X86/ditemplatevalueparameter-remap.ll b/llvm/test/ThinLTO/X86/ditemplatevalueparameter-remap.ll
new file mode 100644
index 00000000000000..76b4b04575bc8c
--- /dev/null
+++ b/llvm/test/ThinLTO/X86/ditemplatevalueparameter-remap.ll
@@ -0,0 +1,92 @@
+; https://github.com/llvm/llvm-project/pull/110064
+; This test case checks if thinLTO correctly links metadata values in a specific
+; situation. Assume we are linking module B into module A, where an extern
+; function used in A is defined in B, but the function body has a
+; DITemplateValueParameter referring to another function back in A. The
+; compiler must check this other function is actually coming from A, thus
+; already materialized and does not require remapping. The IR here is modified
+; from the following source code.
+;
+; // A.h
+; template <void (*Func)()>
+; struct S {
+;   void Impl() {
+;     Func();
+;   }
+; };
+;
+; void func1();
+;
+; // A.cpp
+; #include "A.h"
+; __attribute__((weak)) void func1() {}
+; extern void thinlto1();
+; void bar() {
+;   S<func1> s; // Force instantiation of S<func1> in this compilation unit.
+;   s.Impl();
+;   thinlto1();
+; }
+;
+; // B.cpp
+; #include "A.h"
+; void thinlto1() {
+;   S<func1> s;
+; }
+;
+; RUN: opt -module-summary -o %t1.bc %s
+; RUN: opt -module-summary -o %t2.bc %S/Inputs/ditemplatevalueparameter-remap.ll
+; RUN: llvm-lto2 run %t1.bc %t2.bc -o %t3.o -save-temps \
+; RUN:   -r=%t1.bc,_Z5func1v,p    \
+; RUN:   -r=%t1.bc,_Z3bazv,       \
+; RUN:   -r=%t1.bc,_Z8thinlto1v,  \
+; RUN:   -r=%t1.bc,_Z3barv,px     \
+; RUN:   -r=%t2.bc,_Z8thinlto1v,p
+; RUN: clang -O3 -fthinlto-index=%t3.o.index.bc -x ir %t1.bc -S -emit-llvm -o - | FileCheck %s
+
+target datalayout = "e-m:e-p270:32:32-p271:32:32-p272:64:64-i64:64-i128:128-f80:128-n8:16:32:64-S128"
+target triple = "x86_64-unknown-linux-gnu"
+
+$_Z5func1v = comdat any
+
+define linkonce_odr dso_local void @_Z5func1v() unnamed_addr !dbg !10 {
+  ret void
+}
+
+; Dummy function to use _Z5func1v so that it is not treated as dead symbol.
+define void @_Z3bazv() {
+  tail call void @_Z5func1v()
+  ret void
+}
+
+declare void @_Z8thinlto1v() unnamed_addr
+
+; CHECK: void @_Z3barv()
+; CHECK-NOT: call void @_Z8thinlto1v()
+; CHECK-NEXT: ret void
+define void @_Z3barv() unnamed_addr !dbg !14 {
+  tail call void @_Z8thinlto1v(), !dbg !25
+  ret void
+}
+
+!llvm.dbg.cu = !{!0}
+!llvm.module.flags = !{!2, !3, !4, !5}
+
+!0 = distinct !DICompileUnit(language: DW_LANG_C_plus_plus_14, file: !1, producer: "clang", isOptimized: true, runtimeVersion: 0, emissionKind: FullDebug, splitDebugInlining: false, nameTableKind: None)
+!1 = !DIFile(filename: "A.cpp", directory: ".")
+!2 = !{i32 7, !"Dwarf Version", i32 4}
+!3 = !{i32 2, !"Debug Info Version", i32 3}
+!4 = !{i32 1, !"wchar_size", i32 4}
+!5 = !{i32 8, !"PIC Level", i32 2}
+!10 = distinct !DISubprogram(name: "func1", linkageName: "_Z5func1v", scope: !11, file: !11, line: 6, type: !12, scopeLine: 6, flags: DIFlagPrototyped | DIFlagAllCallsDescribed, spFlags: DISPFlagDefinition | DISPFlagOptimized, unit: !0)
+!11 = !DIFile(filename: "a.h", directory: ".")
+!12 = !DISubroutineType(types: !13)
+!13 = !{null}
+!14 = distinct !DISubprogram(name: "bar", linkageName: "_Z3barv", scope: !11, file: !11, line: 15, type: !12, scopeLine: 15, flags: DIFlagPrototyped | DIFlagAllCallsDescribed, spFlags: DISPFlagDefinition | DISPFlagOptimized, unit: !0, retainedNodes: !16)
+!16 = !{!17}
+!17 = !DILocalVariable(name: "s", scope: !14, file: !11, line: 10, type: !18)
+!18 = distinct !DICompositeType(tag: DW_TAG_structure_type, name: "S<&func1>", file: !11, line: 2, size: 8, flags: DIFlagTypePassByValue, elements: !19, templateParams: !20, identifier: "_ZTS1SIXadL_Z5func1vEEE")
+!19 = !{}
+!20 = !{!21}
+!21 = !DITemplateValueParameter(name: "Func", type: !22, value: ptr @_Z5func1v)
+!22 = !DIDerivedType(tag: DW_TAG_pointer_type, baseType: !12, size: 64)
+!25 = !DILocation(line: 16, column: 5, scope: !14)

[teresajohnson]

I think you will run into trouble with invoking clang here as well. I usually use opt to simulate a distributed ThinLTO backend. Take a look at some of the other test cases in this directory that use llvm-lto2 with -thinlto-distributed-indexes, in particular distributed_import.ll.

[teresajohnson]

As written this will invoke in-process, not distributed, ThinLTO. Which if it reproduces the original failure without your fix is good enough and probably easiest (and you can remove your clang invocation below completely instead of changing it to opt like I suggest there). For distributed thinlto you need to add -thinlto-distributed-indexes. I'm not sure how the test worked without that but maybe the index.bc file used by the clang invocation below was hanging around from a prior invocation when you were using lld?

[teresajohnson]

lgtm thanks