[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/InputFiles.cpp:453
 
+// Linker optimization hints are used for marking a sequence of instructions
+// used for synthesizing an address which that be transformed into a faster
----------------
avoid using 'used for' twice in the same sentence


================
Comment at: lld/MachO/InputFiles.cpp:524
+
+    optimizationHints.push_back({type, {address[0], address[1], address[2]}});
+  }
----------------
how about making `OptimizationHint::offsets` and `address` into `std::array<uint64_t, 3>`? Then we could just use the copy ctor here

edit: just saw thakis' comment about LOH_arm64 below, I guess that would obviate the need for this


================
Comment at: lld/MachO/InputFiles.cpp:573
+
+    for (int i = 0; i < expectedArgCount(hintEnd->type); ++i) {
+      if (LLVM_UNLIKELY(hintEnd->offsets[i] < subsectionBase ||
----------------
just in case the compiler doesn't hoist it out for us


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


================
Comment at: lld/MachO/InputSection.cpp:221
+    if (!optimizationHints.empty())
+      performedRelocs.push_back({r, referentVA, getVA() + r.offset});
   }
----------------
I wonder if it would make the code a bit cleaner to just pass in `isec` to `applyOptimizationHints` instead of computing + storing `relocVA`. Would make it clearer that `applyOptimizationHints` is working on a per-subsection basis at least, plus allocate a bit less memory.

(I suppose we could go one step further and extract `referentVA` from the buffer rather than storing it here, but that might not actually be more efficient...)


================
Comment at: lld/test/MachO/loh-adrp-add.s:4
+# RUN: llvm-mc -filetype=obj -triple=arm64-apple-darwin %s -o %t.o
+# RUN: %lld -arch arm64 %t.o -o %t
+# RUN: llvm-objdump -d --macho %t | FileCheck %s
----------------
we should match against the warning messages too


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

https://reviews.llvm.org/D128093