[PATCH] D120781: [IRLinker] materialize Functions before moving any

[Teresa Johnson via Phabricator via llvm-commits]

tejohnson added inline comments.


================
Comment at: llvm/lib/Linker/IRMover.cpp:1605-1613
+          for (Use &U : I.operands())
+            if (auto *Callee = dyn_cast<Function>(U)) {
+              // TODO: shouldLink?
+              if (!Callee->isMaterializable())
+                continue;
+              if (Error E = Callee->materialize())
+                return E;
----------------
dexonsmith wrote:
> I worry this could cause a big compile-time hit for ThinLTO, depending on the exact code paths it uses.
> 
> Here's part of a testcase for situation I'm worried about:
> ```
> ; f1.ll
> define weak_odr void @f1() {
>   ret void
> }
> 
> ; f2.ll
> define linkonce_odr void @f1() {
>   ret void
> }
> define void @f2() {
>   call @f1()
>   ret void
> }
> ```
> The scenario (more scaffolding needed):
> - Linker decides to take `@f1` from f1.ll and `@f2` from f2.ll.
> - The worklist for `f2.bc` contains just `@f2`, and `@f1` will (never) be materialized from `f2.bc`.
> - IIUC, this patch changes it so that `@f1` is eagerly materialized from `f2.bc` even though it will be ignored.
> 
> In the general case, this would effectively defeat lazy-loading of templates and inline functions.
> 
> @tejohnson , do you know how to set up the above scenario for ThinLTO?
Yes this is a good catch. But I think where it affects issues is a slight change from what you have described above. We don't use the IRMover to load the destination module for each ThinLTO backend, so it wouldn't affect f2.ll when we are importing into it - f2.ll will be parsed here: http://llvm-cs.pcc.me.uk/lib/LTO/LTO.cpp#1088) in the thread for its ThinLTO backend.

However, where it will affect things is when we are importing f2 into another module, when the copy of f1 was selected from f1.ll. We can use llvm-lto2 which is passed explicit linker resolutions to simulate.


```
; f1.ll
target datalayout = "e-m:e-p270:32:32-p271:32:32-p272:64:64-i64:64-f80:128-n8:16:32:64-S128"
target triple = "x86_64-grtev4-linux-gnu"

define weak_odr void @f1() {
  ret void
}

; f2.ll
target datalayout = "e-m:e-p270:32:32-p271:32:32-p272:64:64-i64:64-f80:128-n8:16:32:64-S128"
target triple = "x86_64-grtev4-linux-gnu"

define linkonce_odr void @f1() {
  ret void
}
define void @f2() {
  call void @f1()
  ret void
}

; f3.ll
target datalayout = "e-m:e-p270:32:32-p271:32:32-p272:64:64-i64:64-f80:128-n8:16:32:64-S128"
target triple = "x86_64-grtev4-linux-gnu"

declare void @f2()
define void @f3() {
  call void @f2()
  ret void
}
```

```
$ opt -module-summary f1.ll -o f1.o
$ opt -module-summary f2.ll -o f2.o
$ opt -module-summary f2.ll -o f2.o
$ llvm-lto2 run f1.o f2.o f3.o -save-temps -o f -r=f1.o,f1,plx -r=f2.o,f1, -r=f2.o,f2,plx -r=f3.o,f2, -r=f3.o,f3,plx -debug-only=function-import
```

The end of the debug output shows what we import from where into f3.o:

```
Is importing function 2072045998141807037 f1 from f1.ll
Not importing function 2072045998141807037 f1 from f2.ll
Is importing function 8471399308421654326 f2 from f2.ll
Imported 2 functions for Module f3.o
```

So we should not materialize f1 from f2.o when importing in f3.o's ThinLTO backend thread.

However, as noted in one of the comments earlier, the function importer has an empty AddLazyFor callback, so we will never add new values to materialize during the value mapping. I wonder if we key off of this somehow to not attempt to add new functions to materialize here? 


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D120781/new/

https://reviews.llvm.org/D120781