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

[Daniel Bertalan via Phabricator via llvm-commits]

BertalanD added inline comments.


================
Comment at: lld/MachO/Arch/ARM64.cpp:214-215
+    return;
+  if (adrp.destRegister != add.srcRegister)
+    return;
+
----------------
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?


================
Comment at: lld/MachO/InputFiles.cpp:453
 
+void ObjFile::parseOptimizationHints(ArrayRef<uint8_t> data) {
+  auto expectedArgCount = [](uint8_t type) {
----------------
int3 wrote:
> can you include a comment block explaining what optimization hints are? something like what you put down in the commit message would be a good. Would also love an explanation of how `minOffset` is important.
> 
> we don't always have the most detailed comments, but if you check the top of InputFiles.cpp, there's a pretty detailed comment about the Mach-O format -- that's a good ideal to strive for :)
Comments are a good idea (especially since this feature is a bit obscure -- I personally didn't know about it before I looked through the GitHub issues).

I'm going to add a general description to this function. Additionally, I'll explain each kind individually in the corresponding `performFoo` function.


================
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.
----------------
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.


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

https://reviews.llvm.org/D128093