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

[Peter Smith via Phabricator via llvm-commits]

peter.smith added a comment.

Thanks for working on this. A couple of comment suggestions. I may have spotted a problem with the ADD encoding (missing src register) but that could be me misreading the code.

I'm on vacation for the next couple of weeks so may be a bit slow replying.



================
Comment at: lld/ELF/Arch/AArch64.cpp:571
 
+AArch64Relaxator::AArch64Relaxator(ArrayRef<Relocation> relocs) {
+  if (config->emachine != EM_AARCH64) {
----------------
I'd recommend AArch64Relaxer rather than AArch64Relaxator as relaxer is commonly used for something that relaxes something, for example muscle relaxer, hair relaxer. 


================
Comment at: lld/ELF/Arch/AArch64.cpp:610
+
+  // TODO: Add a link to the ARM 64-bit ABI specification.
+  // Check if the relocations have the expected types.
----------------
In lieu of the ABI would it be worth something like
// When the definition of sym is not preemptible then we may
// be able to relax
// ADRP xn, :got: sym
// LDR xn, [ xn :got_lo12: sym]
// to
// ADRP xn, sym
// ADD xn, xn, :lo_12: sym


================
Comment at: lld/ELF/Arch/AArch64.cpp:622
+  Symbol &sym = *adrpRel.sym;
+  // Skip undefined, preemtible and STT_GNU_IFUNC symbols.
+  if (sym.isUndefined() || sym.isPreemptible || sym.isGnuIFunc())
----------------
typo: preemptible


================
Comment at: lld/ELF/Arch/AArch64.cpp:634
+  // Check if the second instruction is LDR.
+  if ((ldrInstr & 0x3B000000) != 0x39000000)
+    return false;
----------------
Although it shouldn't happen in practice as I'm not sure if even ILP32 loads 32-bit values from the GOT, it will be worth checking that the `sf` bit 31 is set to 1 (for a 64-bit load) as the ADD instruction we are constructing later has sf set to 1.


================
Comment at: lld/ELF/Arch/AArch64.cpp:664
+            0x90000000 | adrpDestReg /* adrp x_<dest_reg> */);
+  write32le(buf + addRel.offset,
+            0x91000000 | adrpDestReg /* add x_<dest reg> */);
----------------
Unless I've missed something this looks like we're missing the source register for the ADD (bits 5 -9) I think. I notice all the successful tests are using x0 as the source and destination register so this could go undetected as it will default to 5-9 set to 0 for x0.


================
Comment at: lld/test/ELF/aarch64-adrp-ldr-got.s:1
+# REQUIRES: aarch64
+# RUN: split-file %s %t
----------------
Can we add a test case that does the relaxation but uses something other than X0 as the ADRP and LDR destination. We may miss problems with the encodings as writing zeroes will use X0?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D112063