[PATCH] D128601: [ORC][ORC_RT][AArch64] Implement TLS descriptor in ELFNixPlatform.

[Lang Hames via Phabricator via llvm-commits]

lhames added a comment.

Amazing -- TLV was supposed to be a stretch goal. ;)



================
Comment at: compiler-rt/lib/orc/elfnix_tls.aarch64.S:20
+
+  // returns address of TLV in x0, all other registers preserved
+  .globl ___orc_rt_elfnix_tlsdesc_resolver
----------------
Could this re-entry code have a fast-path for instances once they've been allocated (as in https://github.com/llvm/llvm-project/issues/51162, though obviously the specifics would be different for TLSDESC). If so it's probably worth a comment here "// TODO: add fast-path for repeat access".


================
Comment at: llvm/include/llvm/ExecutionEngine/JITLink/aarch64.h:25
 enum EdgeKind_aarch64 : Edge::Kind {
-  Branch26 = Edge::FirstRelocation,
+  Nop = Edge::FirstRelocation,
+  Branch26,
----------------
Is `Nop` actually needed here, or could we just not introduce the edge?


================
Comment at: llvm/include/llvm/ExecutionEngine/JITLink/aarch64.h:34-35
   GOTPageOffset12,
-  TLVPage21,
-  TLVPageOffset12,
   PointerToGOT,
----------------
I think that these (`TLVPage21` and `TLVPageOffset12`) should stay in: The idea is to have the edge kinds in the initially parsed graph be very descriptive (e.g. "I am a page21-expr offset _to a TLV pointer_") so that early passes have a clear picture of what the graph represents.



================
Comment at: llvm/include/llvm/ExecutionEngine/JITLink/aarch64.h:226-228
+  case Nop: {
+    break;
+  }
----------------
Braces aren't needed here, since no new decls are introduced.


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

https://reviews.llvm.org/D128601