[PATCH] D117614: [lld][ELF] Add support for ADRP+ADD optimization for AArch64

[Fangrui Song via Phabricator via llvm-commits]

MaskRay added a comment.

Mostly good with some nits. Thanks for bearing with me:)



================
Comment at: lld/ELF/Arch/AArch64.cpp:597
+                                     uint8_t *buf) const {
+  // When the address of sym is within the range of ADR then
+  // we may relax
----------------
`// When the address of sym is within the range of ADR, we may relax`


================
Comment at: lld/test/ELF/aarch64-adrp-add.s:52
+# RUN: ld.lld %t/a.o -T %t/within-adr-range-low.t --no-relax -o %t/a
+## The option --no-relax is specified, no relaxations should be applied.
+# RUN: llvm-objdump --no-show-raw-insn -d %t/a | FileCheck %s --check-prefix=OUT-OF-RANGE
----------------
The comment can be made conciser: `## --no-relax disables relaxation.`


================
Comment at: lld/test/ELF/aarch64-adrp-add.s:55
+
+## This linker script ensures that .rodata and .text are close to each other,
+## the adrp + add pair can be relaxed to nop + adr, moreover, the address difference
----------------
`This linker script ensures that ` can be omitted: `## .rodata and .text are close to each other. xxxx`

It is clear that this is a linker script. Making the comment shorter saves readers' time.


================
Comment at: lld/test/ELF/aarch64-adrp-add.s:57
+## the adrp + add pair can be relaxed to nop + adr, moreover, the address difference
+## is equal to the lowest allowed value.
+#--- within-adr-range-low.t
----------------
equals the lowest allowed value (-1MiB).

Add more information to "lowest allowed value"


================
Comment at: lld/test/ELF/aarch64-adrp-add.s:78
+SECTIONS {
+ .rodata 0x101003: { *(.rodata) }
+ .text   0x1000: { *(.text) }
----------------
Decreasing addresses is not well modeled/supported in ld.lld. Try avoiding this. You can move .rodata after .text


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D117614