[PATCH] D144083: [JITLink] Initial 32-bit ARM backend
Peter Smith via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Wed Feb 15 10:45:32 PST 2023
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
More information about the llvm-commits
mailing list