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

[Duncan P. N. Exon Smith via Phabricator via llvm-commits]

dexonsmith added a comment.

In D120781#3353199 <https://reviews.llvm.org/D120781#3353199>, @nickdesaulniers wrote:

> In D120781#3353144 <https://reviews.llvm.org/D120781#3353144>, @dexonsmith wrote:
>
>> Thanks for working on this; it's pretty hairy.
>>
>>> This confuses BitcodeReader, which cannot disambiguate whether a
>>> blockaddress is referring to a function which has not yet been parsed
>>> ("materialized") or is simply empty because its body was spliced out.
>>
>> Another way to solve this would be to mark the function somehow (either intrusively, somehow, or by adding it to a set / map). Can you contrast with that approach?
>
> BitcodeReader could maintain a set of all functions it's imported, so that it could do the right thing here (rather than assume a function being empty means not yet parsed);
> https://github.com/llvm/llvm-project/blob/1e16272ba793e5a6e7308898ecf6ef0dc99e2ad3/llvm/lib/Bitcode/Reader/BitcodeReader.cpp#L3037-L3066

Can it also check `isMaterializable()` to differentiate? Is the "materializable" bit reset after materializing, allowing us to differentiate between "empty becuase it's materializable" and "empty because content got moved away"?

Not entirely sure this'll work, because of the cycle case (testcase `@c` and `@d`)... but maybe if you know it's been moved, there's a way to look up the location of the new definition? (is the valuemapper available?)

(If this isn't going to materialize anything *extra*, it seems fine to materialize in an earlier loop (your patch).)

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

  define void @sink(i8*)
  define linkonce_odr void @haslabel() {
    br label %label
  label:
    ret void
  }
  define void @usesblockaddress() {
    call void @sink(i8* blockaddress(@haslabel, %label))
    ret void
  }

What if this TU's `@haslabel` is not selected by the linker? (Maybe this is just a failed link, since there's no guarantee the other `@haslabel` still has a basic block, or that it means the same thing, since it could be optimized away if its TU didn't have a `blockaddress` pointing at it.)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D120781