[PATCH] D112063: [lld][ELF] Add first bits to support relocation relaxations for AArch64

[Alexander Shaposhnikov via Phabricator via llvm-commits]

alexander-shaposhnikov marked 2 inline comments as done.
alexander-shaposhnikov added inline comments.


================
Comment at: lld/ELF/Arch/AArch64.cpp:572
+AArch64Relaxer::AArch64Relaxer(ArrayRef<Relocation> relocs) {
+  if (!config->relax || config->emachine != EM_AARCH64) {
+    safeToRelaxAdrpLdr = false;
----------------
MaskRay wrote:
> The condition `config->emachine != EM_AARCH64` is unneeded. It may be needed with your unposted dependented patches. This is one reason I suggest that you post the dependent patch together.
this is needed in the following sense - for other architectures we avoid doing unnecessary work below


================
Comment at: lld/ELF/Arch/AArch64.cpp:648
+
+  Relocation adrpSymRel = {
+      R_AARCH64_PAGE_PC,          // expr
----------------
MaskRay wrote:
> In other replaces we just use the concise form to initialization a `Relocation`
> 
> ```
>    Relocation adrpRel{R_AARCH64_PAGE_PC, R_AARCH64_ADR_PREL_PG_HI21,
>                       adrpRel.offset, 0, &sym};
> ```
> I assume that `adrpSymRel` can be named to `adrpRel`
honestly I find the current version easier to read than the concise one (e.g.  what does 0 mean there ?  one has to look it up and memorize) 


================
Comment at: lld/test/ELF/aarch64-adrp-ldr-got.s:9
+
+## Case 1. Symbol 'x' is nonpreemptible, the relaxation should be applied.
+## This test verifies the encoding when the register x1 is used.
----------------
MaskRay wrote:
> Avoid `Case 1. `. New cases may be inserted between two existing one and the case number would just cause unneeded number adjustment.
Yeah, they may be inserted, but somehow I find it convenient when the cases are numbered (and people read the code more often than they modify it). Quick grepping the LLVM repository shows that there are other places with such comments (not mine).


================
Comment at: lld/test/ELF/aarch64-adrp-ldr-got.s:20
+# RUN: ld.lld -shared %t-load-to-x1.o -T %t/linker.script --no-relax -o %t-load-to-x1.so
+# RUN: llvm-objdump --no-show-raw-insn -d %t-load-to-x1.so \
+# RUN: | FileCheck --check-prefix=NO-RELAX %s
----------------
MaskRay wrote:
> Indent the continuation line. My preferred style (and jhenderson's) is
> ```
> # RUN: llvm-objdump --no-show-raw-insn -d %t-load-to-x1.so | \
> # RUN:   FileCheck --check-prefix=NO-RELAX %s
> ```
> 
> When split-file and %t are used, I'd place all temporary files (including executables) under %t, too. So that you can delete all temporary files at once.
ok


================
Comment at: lld/test/ELF/aarch64-adrp-ldr-got.s:27
+## Case 3. LDR and ADRP use different registers, no relaxations should be applied.
+# RUN: ld.lld %t-mismatched-src-reg.o -o %t-mismatched-src-reg.exe
+# RUN: llvm-objdump --no-show-raw-insn -d %t-mismatched-src-reg.exe \
----------------
MaskRay wrote:
> For executables, just omit the suffix name. Avoid `.exe`
I'm curious - is this documented anywhere or is it the local code style adopted by LLD? 
having .exe is convenient since it gives a hint to the reader / or the person who runs the test that this particular file is an executable (or at least intended to be)  - grepping the LLVM repository returns a huge number of places (tests) where this is used.  I do not mind dropping it here - but really curious about the rationale for it.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D112063