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

Mon Mar 14 10:46:09 PDT 2022

nickdesaulniers added a comment.

In D120781#3376069 <https://reviews.llvm.org/D120781#3376069>, @dexonsmith wrote:

> - The only way to avoid a materialization explosion is to record backedges.

Unless I can make @tejohnson 's suggestion work regarding making `IRLinker::AddLazyFor` optional, then at that point I agree.

> (Sorry for the poor guidance!)

No, no! I appreciate the feedback; it's helpful to have folks engage on this issue.

> Here's what I suggest:
>
> - Add to bitcode a way to say that materializing a function requires materializing another one. Maybe this could be a new record in the `FUNCTION_BLOCK` that's arbitrary size, only emitted if there are non-zero of them, which lists the functions that need to be materialized when this one is.
> - Use this feature to add edges in both directions every time one function has a `blockaddress` to another one. Probably not too hard; during value enumeration of function bodies, create a map of which functions need to be materialized with each other (based on observed `blockaddress`es), then use that to generate the records during bitcode emission.
> - Change the lazy materializer to use this information somehow (maybe this is the hard part).
>
> WDYT?

I can give it a shot.

================
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;
----------------
tejohnson wrote:
> 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? 
> So we should not materialize f1 from f2.o when importing in f3.o's ThinLTO backend thread.

Ah, thanks for the test case. My current approach (Diff 414166) does materialize f1 from f2.o, so that would be a regression.

> 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?

So if we made `IRLinker::AddLazyFor` an `Optional` function reference (or function pointer) then we could easily tell+guarantee that no new functions could be added lazily to `IRLinker::Worklist`.  Then we could go back to an earlier+simpler diff <https://reviews.llvm.org/D120781?id=412277> just wrapped in one conditional check that `AddLazyFor` was not available.  That might be the smallest possible fix that might be possible to get into the clang-14 release.

Let me play with that approach a little before pursuing @dexonsmith 's recommendation to encode "backedges" (references) to functions in the bitcode, and let's see what that looks like if it works.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D120781