[PATCH] D128093: [lld-macho] Initial support for Linker Optimization Hints

[Jez Ng via Phabricator via llvm-commits]

int3 added inline comments.


================
Comment at: lld/MachO/Arch/ARM64.cpp:214-215
+    return;
+  if (adrp.destRegister != add.srcRegister)
+    return;
+
----------------
BertalanD wrote:
> int3 wrote:
> > do we ever expect an optimization hint to be emitted for instructions whose src/dest don't match? does ld64 perform this same validation?
> > 
> > ditto for the referentVA check below
> ld64 checks these conditions and emits a warning if they don't hold. I assume it means that these are expected to be true for valid objects.
> 
> I tested it with this file:
> ```
> .align 2
> .globl _main
> _main:
> L1:
>   adrp x0, _foo at PAGE
> L2:
>   add x1, x1, _foo at PAGEOFF
> L3:
>   add x0, x0, _bar at PAGEOFF
> L4:
>   nop
> .loh AdrpAdd L1, L2
> .loh AdrpAdd L1, L3
> .loh AdrpAdd L1, L4
> _foo:
>   .long 0
> _bar:
>   .long 0
> ```
> 
> This is ld64's output:
> ```
> ld: warning: ignoring linker optimization hint at _main+0x0 because adrpInfoA.destReg == addInfoB.srcReg
> ld: warning: ignoring linker optimization hint at _main+0x0 because infoA.target == infoB.target
> ld: warning: ignoring linker optimization hint at _main+0x0 because isPageOffsetKind(infoB.fixup)
> ```
> 
> 
> Do you think we should add similar diagnostics?
Thanks for checking!

> Do you think we should add similar diagnostics?

Wouldn't hurt, but I wouldn't consider it high pri either

We *should* add test cases that cover these code paths though


================
Comment at: lld/MachO/InputFiles.cpp:492-493
+      if (expectedCount == -1) {
+        warn("Linker optimization hint at offset " + Twine(inputOffset) +
+             " has unknown type " + Twine(type));
+        // Skip it.
----------------
BertalanD wrote:
> int3 wrote:
> > do we actually expect this to happen in practice? can we just error out and return early?
> I added it for forward compatibility.
> 
> However, it looks like the LLVM pass that emits these hints hasn't been changed substantially for a long time, so it's probably safe to say that we won't be getting any new types that will be need to handled.
Gotcha. I would also argue that we should expect people to upgrade LLD and clang in lockstep most of the time, so we don't need to worry too much about forwards compatibility


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

https://reviews.llvm.org/D128093