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

Mon Mar 14 13:25:26 PDT 2022

nickdesaulniers added a comment.

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

> Another potential concern: what happens with weak linkage? I.e., what if you have:

Thinking about this over the weekend; I don't think we should allow `blockaddress`es to reference `available_externally` linkage functions if the `blockaddress`' `Function` differs from the `Instruction`'s `Function` (for the `Instruction` with the `blockaddress` `Constant` operand).

For example:

  ; y.ll
  declare void @f(i8*)

  define available_externally void @x() {
    br label %label

  label:
    call void @y()
    ret void
  }

  define void @y() {
    ; Note: blockaddress refers to @x in fn @y.
    call void @f(i8* blockaddress(@x, %label))
    ret void
  }

  $ llc -o - y.ll

  	.text
  	.file	"y.ll"
  	.globl	y                               # -- Begin function y
  	.p2align	4, 0x90
  	.type	y, at function
  y:                                      # @y
  	.cfi_startproc
  # %bb.0:
  	pushq	%rax
  	.cfi_def_cfa_offset 16
  	movl	$.Ltmp0, %edi ; oops! where is .Ltmp0?
  	callq	f at PLT
  	popq	%rax
  	.cfi_def_cfa_offset 8
  	retq
  .Lfunc_end0:
  	.size	y, .Lfunc_end0-y
  	.cfi_endproc
                                          # -- End function
  	.section	".note.GNU-stack","", at progbits

  $ opt -verify -S y.ll -o /dev/null
  $ echo $?
  0

Perhaps I should think harder about additional linkages that are added lazily to `IRLinkers`'s `Worklist` (like `linkonce_odr` and perhaps `weak`).  If all of the cases that could be added lazily are made invalid IR, with new verifier checks added, we could update IPSCCP to not sink such `blockaddress` `Constant`s that would produce such invalid IR.  Let me see if a quick verifier check can find any existing test cases I'm not thinking of where that might not work.

================
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;
----------------
nickdesaulniers wrote:
> 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.
> 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.

That might work for thinLTO, but it doesn't for the simple test case using `llvm-link` added in this patch; `IRLinker::AddLazyFor` is defined. :( https://reviews.llvm.org/D121630 is still a worthwhile cleanup IMO.

I'll start looking into bitcode modifications.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D120781