[PATCH] D112451: implemented R_AARCH64_ADR_PREL_PG_HI21, R_AARCH64_ADR_PREL_PG_HI21_NC, R_AARCH64_ADD_ABS_LO12_NC

[Stefan Gränitz via Phabricator via llvm-commits]

sgraenitz added inline comments.


================
Comment at: llvm/lib/ExecutionEngine/JITLink/ELF_aarch64.cpp:103-105
+  template <uint8_t hi, uint8_t lo> static uint32_t extractBit(uint64_t num) {
+    return (static_cast<uint32_t>(num) & ((1 << (hi + 1)) - 1)) >> lo;
+  }
----------------
lhames wrote:
> Do we have any existing bit-slicing utilities like this? It feels like we should, but I don't actually recall seeing them anywhere, and I don't think I've used anything like them in the MachO implementation.
> 
> It would be good to check. If there are existing ones we should use them. If there aren't then we should aim to move these into a place where they can be re-used (that can be done separately from this patch).
Not that I am aware of. Agree to put a little more effort into it. In fact, for range checks, both RuntimeDyld and LLD make frequent use of `SignExtend64` from `llvm/Support/MathExtras.h`. 

Looking at the LLD code, it mostly uses raw shifts and typically it won't extract bits. Instead it tends to keep existing bits untouched and just overwrites the ones it needs to modify. I think this approach would work here as well. If so, we probably should get rid of `extractBit` altogether.


================
Comment at: llvm/lib/ExecutionEngine/JITLink/ELF_aarch64.cpp:107-109
+  constexpr JITTargetAddress getPageAddress(JITTargetAddress expr) const {
+    return expr & ~0xfff;
+  }
----------------
lhames wrote:
> Are arm64 pages always 4k on Linux? On Darwin they're 16k.
Can we access the actual page size reported from the executor? `ExecutorProcessControl` has it and we could make it accessible from here either via JITLinkContext or JITLinkMemoryManager. Both appear out of scope for this patch, but we should at least add a FIXME here.


================
Comment at: llvm/lib/ExecutionEngine/JITLink/ELF_aarch64.cpp:111-113
   template <uint8_t Bits> static bool fitsRangeSignedInt(int64_t Value) {
     return Value >= -(1 << Bits) && Value < (1 << Bits);
   }
----------------
lhames wrote:
> This should be moved into the aarch64.h header and renamed `isInRangeForImmSN` for consistency with the existing x86_64 functions.
Yes it has been part of my initial patch. I will move it as suggested or remove it if we go the `SignExtend` way.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D112451