[PATCH] D147544: [BOLT] Move from RuntimeDyld to JITLink

[Amir Ayupov via Phabricator via llvm-commits]

Amir added a comment.

In D147544#4291902 <https://reviews.llvm.org/D147544#4291902>, @jobnoorman wrote:

> In D147544#4288096 <https://reviews.llvm.org/D147544#4288096>, @rafauler wrote:
>
>> We've dealt with large changes to BOLT a few times before, and the easiest way to guarantee that we're not missing anything was always to change the code until it matches the binaries, so we're fully aware of all of the changes that are going in production.
>
> Makes sense. I got the patch in a state where it now produces 100% matching binaries on all test cases (with a few tweaks, see below).
>
> In D147544#4288256 <https://reviews.llvm.org/D147544#4288256>, @rafauler wrote:
>
>> I think I forgot to clarify this: when I mentioned nondeterminism when running the terminator insertion pass, I meant same binary, same inputs, same everything, but getting different outputs from run to run.
>
> You were right that using the terminator insertion pass should make this patch match the current behavior of BOLT. The non-determinism you observed was caused by the blocks of sections being stored in a `DenseSet` in `JITLink`. I fixed this by ordering blocks by address when iterating.
>
>> But another source of non-determinism that we can't have is iteration over DenseMaps, because those change if we build a new version of BOLT and cause a bunch of headaches when we are qualifying a new release.
>
> I discovered that `JITLink` keeps track of the creation order of sections so this source of non-determinism has also been fixed now.
>
> To make binaries produced by this patch match the ones BOLT currently produces, a few extra tweaks are necessary. I've attached them in a separate patch because I believe they should only be necessary for validation purposes. This patch does the following:
>
> - Switch the order of `.bss.bolt.extra` and `.data.bolt.extra` sections. For some reason, `RuntimeDyLd` did not allocate those sections in the same order as they are in the input file.
> - Use 1-byte allocations for empty sections. `RuntimeDyld` forces the minimum allocation size to 1 byte. I believe it should be fine to skip this and simply allocate an empty section. (This happens with empty `.text` or `.text.cold` sections in `X86/bb-with-two-tail-calls.s` and `X86/bug-reorder-bb-jrcxz.s`.)
> - Explicitly ignore `.note.gnu.property` sections. This section comes from `instr.cpp.o` and is marked as allocatable in the ELF file but was still skipped by `RuntimeDyld`.
> - Add `.eh_frame` terminator passes.
>
> F27232098: 0001-RuntimeDyld-simulation.patch <https://reviews.llvm.org/F27232098>
>
> To make all binaries match completely deterministically, the reference version of BOLT also needs to be tweaked, unfortunately: when `RuntimeDyld` forces the minimum allocation to 1 byte (as explained above), it doesn't actually initialize that byte. This causes the section contents of empty sections to sometimes mismatch. The following patch fixes this.
>
> F27232136: 0001-Make-RuntimeDyld-deterministic.patch <https://reviews.llvm.org/F27232136>

Thanks for the second patch, we can incorporate it since it addresses an existing non-determinism in BOLT (see https://github.com/llvm/llvm-project/issues/59008)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D147544