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

[Jez Ng via Phabricator via llvm-commits]

int3 added a comment.

> The added overhead on linking Chromium is 200 milliseconds on my M1 <https://reviews.llvm.org/M1> Mac mini, which accounts for about 4% of the total runtime.

Is there measurable overhead when `-ignore_optimization_hints` is passed? I'm wondering about the overhead from increasing the Reloc struct size

4% overhead is not such a big deal IMO if we can disable it in developer builds (though the fact that it's enabled by default is unfortunate)



================
Comment at: lld/MachO/Arch/ARM64.cpp:167
+
+static bool parseADRP(uint32_t insn, ADRP &adrp) {
+  if ((insn & 0x9f000000) != 0x90000000)
----------------
nit: I would prefer `parseAdrp` (would also be more consistent with `applyAdrpAdrp` below)


================
Comment at: lld/MachO/Arch/ARM64.cpp:184
+static void writeADR(void *loc, uint32_t dest, int32_t delta) {
+  uint32_t instruction = 0x10000000;
+  uint32_t immHi = (delta & 0x001ffffc) << 3;
----------------
`op` or `opcode` would be more accurate


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


================
Comment at: lld/MachO/InputFiles.cpp:453
 
+void ObjFile::parseOptimizationHints(ArrayRef<uint8_t> data) {
+  auto expectedArgCount = [](uint8_t type) {
----------------
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 :)


================
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.
----------------
do we actually expect this to happen in practice? can we just error out and return early?


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

https://reviews.llvm.org/D128093