[PATCH] D148701: [LLD][ELF][AArch64] Add AArch64 short range thunk support

[Fangrui Song via Phabricator via llvm-commits]

MaskRay accepted this revision.
MaskRay added a comment.
This revision is now accepted and ready to land.

Thanks for the update. With the base class, the number of lines have decreased by 4 even with the long comment "An AArch64 thunk may be either short or long ...", so the base class looks worthwhile!



================
Comment at: lld/ELF/Thunks.cpp:57
 public:
-  AArch64ABSLongThunk(Symbol &dest, int64_t addend) : Thunk(dest, addend) {}
-  uint32_t size() override { return 16; }
+  bool getMayUseShortThunk();
+  AArch64Thunk(Symbol &dest, int64_t addend) : Thunk(dest, addend) {}
----------------
It seems more conventional to place constructors before member functions.


================
Comment at: lld/test/ELF/aarch64-long-thunk-converge.s:6
+// RUN: llvm-objdump --no-show-raw-insn -d --start-address=0x7001004 --stop-address=0x7001010 %t/a | FileCheck %s
+// RUN: llvm-objdump --no-show-raw-insn -d --start-address=0x11001008 --stop-address=0x11001014 %t/a | FileCheck --check-prefix=CHECK2 %s
+
----------------
Perhaps remove `%t/a` after the link. It is very large (420MiB).


================
Comment at: lld/test/ELF/aarch64-long-thunk-converge.s:8
+
+/// This test shows that once a long-thunk has been generated it
+/// cannot be written as a short thunk. This prevents oscillations
----------------
The paragraphs uses several spellings for a long thunk: long-thunk, long-range thunk, and long range thunk. Canonicalize them?


================
Comment at: lld/test/ELF/aarch64-long-thunk-converge.s:11
+/// in size that can prevent convergence.
+/// In pass 1 the distance between bl foo and foo: requires a long-range thunk
+/// In pass 2 the long range thunk also inserted in pass 1 before foo:
----------------
Our `pass` variable starts with 0.

/// In pass 0, `bl foo` requires a long-range thunk to reach foo. The thunk for bar increases the address of foo so that it can be reached by `bl foo` with a single b instruction.
/// In pass 1, we expect the long-range thunk for `bl foo` to remain long.


================
Comment at: lld/test/ELF/aarch64-long-thunk-converge.s:16
+
+// CHECK: <__AArch64ADRPThunk_>:
+// CHECK-NEXT: 7001004: adrp    x16, 0x11001000
----------------



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

https://reviews.llvm.org/D148701