[PATCH] D144083: [JITLink] Initial 32-bit ARM backend

[Peter Smith via Phabricator via llvm-commits]

peter.smith added a comment.

I've taken a look from the Arm side, for initial support I think a lot of the comments don't need attention, but may be worth considering for future revisions. While what's here looks similar to other JITLink backends, I don't have any experience with it so I'll need to defer to someone else more familiar with its code base. Very happy to look at the Arm specifics though.

I must confess that I haven't double checked all the bit-patterns for the relocations against the Arm ARM, the bits that I can easily cross reference from LLD look like they match.



================
Comment at: llvm/include/llvm/ExecutionEngine/JITLink/arm.h:33
+  /// Plain 32-bit pointer relocation; optional addend is stored in-place
+  Arm_Delta32 = FirstArmRelocation,
+
----------------
Strictly speaking that would be a Data relocation (https://github.com/ARM-software/abi-aa/blob/main/aaelf32/aaelf32.rst#5613static-data-relocations) rather than an Arm state relocation.

Not sure if you are planning Arm state instruction support, may be worth making the distinction if you are.


================
Comment at: llvm/include/llvm/ExecutionEngine/JITLink/arm.h:59
+
+/// Implementation of ARM branch range extension stubs ("veneers") vary with
+/// ISA kinds (arm/thumb/interworking), CPU architectures (v4, v6, v7) and
----------------
Not a strong opinion, but you may wish to use stubs throughout the implementation as I think JITLink seems to use stubs.

Veneers, Stubs and Thunks are all synonyms and although the Arm ABI calls them veneers GNU ld and LLD stick to the linker's choice.


================
Comment at: llvm/include/llvm/ExecutionEngine/JITLink/arm.h:108
+/// Returns extracted bits Val[Hi:Lo].
+inline uint32_t extractBits(uint32_t Val, unsigned Hi, unsigned Lo) {
+  return (Val & (((1UL << (Hi + 1)) - 1))) >> Lo;
----------------
IIUC this might be undefined behaviour for a 32-bit type if Hi is 31 as we'll end up with (1UL << 32) -1. Did you mean to make Val uint64_t?


================
Comment at: llvm/include/llvm/ExecutionEngine/JITLink/arm.h:129
+      return makeTargetOutOfRangeError(G, B, E);
+    *(little32_t *)FixupPtr = Value;
+    break;
----------------
I guess we are only supporting little-endian for now (I don't blame you!) while for v7/v8 all instructions will always be little-endian the data can be big-endian on a big-endian system.


================
Comment at: llvm/include/llvm/ExecutionEngine/JITLink/arm.h:140
+  case Thumb_Call: {
+    int64_t Addend = -getPCBias(E.getKind());
+    int64_t Value = TargetAddress - FixupAddress + Addend;
----------------
It looks like this currently only handles Thumb destinations, i.e. it doesn't write a BLX instruction to Arm state if the TargetAddress has the bottom bit set.

Is this because all Arm/Thumb state transitions are veneered so we never get an opportunity to use BLX?


================
Comment at: llvm/include/llvm/ExecutionEngine/JITLink/arm.h:211
+
+  static StringRef getSectionName() { return "Veneer$$Code"; }
+
----------------
The convention for the other targets seems to be "S__STUBS" I know armlink Arm's proprietary linker uses Veneer$$Code, but may be worth keeping in sync with other JITLink targets.


================
Comment at: llvm/include/llvm/ExecutionEngine/JITLink/arm.h:218
+    switch (E.getKind()) {
+    case arm::Thumb_Call:
+    case arm::Thumb_Jump24: {
----------------
Is this indirecting all jumps and calls through a veneer? One thing a static linker needs to be careful about is functions that do not have type STT_FUNC (https://github.com/ARM-software/abi-aa/blob/main/aaelf32/aaelf32.rst#553symbol-values). In this case the bottom bit is alway expected to be 0, even for Thumb Targets. The current veneer would attempt to switch to Arm state for a relocation with bit 0 clear.

Non STT_FUNC symbols are expected to represent labels inside a function so they are always the same state.

It maybe that the JITLink doesn't encounter non STT_FUNC symbols at link time as it should be possible to resolve branches to internal labels at fixup time without having to expose them to the linker.


================
Comment at: llvm/lib/ExecutionEngine/JITLink/ELF_arm.cpp:177
+  case v8_A:
+    assert(getArmConfigForCPUArch(Arch).Veneers != arm::Unsupported &&
+           "Provide a ARM config for each supported CPU");
----------------
v7 without the profile attribute includes v7-m, v7-a and v7-r. If this is a problem then I recommend checking Tag_CPU_arch_profile (https://github.com/ARM-software/abi-aa/blob/main/addenda32/addenda32.rst#335target-related-attributes) and checking for A profile.

In terms of instructions v7, v7E_M, v8_M_Main, and v8-R will be compatible, although they are not likely targets for JITLink.

v9_A is also a superset of v8_A so you could include support that with no extra cost. 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D144083